The whole app shipped in one 578 KB chunk, so a phone on cell data downloaded and parsed everything — the canvas editor, gestures, geometry, every page — before the login screen could paint (the >500 KB Vite warning).
What
Lazy-load the heavy/deep routes via React.lazy: the editor (its GardenCanvas + @use-gesture + geometry are the biggest surface), the public garden view, plants, settings, register. Login and the gardens list stay eager (entry points — no fallback flash on landing). AppShell wraps <Outlet> in a Suspense boundary.
One vendor manualChunk for all node_modules, so the rarely-changing libraries cache across app deploys while the tiny app chunk churns. Kept as a single chunk deliberately: splitting react-dom/scheduler into their own chunk reorders module init across chunk boundaries and breaks React 19 at load (Cannot set 'Activity' of undefined) — I hit exactly that, verified it, and backed it out (the comment records why).
Result (measured)
before
after
app entry chunk
578 KB
39 KB
vendor (cached across deploys)
—
421 KB
editor (GardenEditorPage+GardenCanvas)
in entry
60 KB, lazy
No more >500 KB warning.
Verified live (embedded binary)
/gardens loads with only index+vendor; opening a garden lazy-fetches the editor chunk and renders the canvas; console clean; the Go embed.FS serves the hashed split chunks and the SPA fallback correctly.
Closes #106. Part of #96.
The whole app shipped in one **578 KB** chunk, so a phone on cell data downloaded and parsed everything — the canvas editor, gestures, geometry, every page — before the login screen could paint (the `>500 KB` Vite warning).
## What
- **Lazy-load the heavy/deep routes** via `React.lazy`: the editor (its `GardenCanvas` + `@use-gesture` + geometry are the biggest surface), the public garden view, plants, settings, register. Login and the gardens list stay **eager** (entry points — no fallback flash on landing). `AppShell` wraps `<Outlet>` in a `Suspense` boundary.
- **One `vendor` manualChunk** for all `node_modules`, so the rarely-changing libraries cache across app deploys while the tiny app chunk churns. Kept as a **single** chunk deliberately: splitting react-dom/scheduler into their own chunk reorders module init across chunk boundaries and **breaks React 19 at load** (`Cannot set 'Activity' of undefined`) — I hit exactly that, verified it, and backed it out (the comment records why).
## Result (measured)
| | before | after |
|---|---|---|
| app entry chunk | 578 KB | **39 KB** |
| vendor (cached across deploys) | — | 421 KB |
| editor (`GardenEditorPage`+`GardenCanvas`) | in entry | **60 KB, lazy** |
No more `>500 KB` warning.
## Verified live (embedded binary)
`/gardens` loads with only `index`+`vendor`; opening a garden lazy-fetches the editor chunk and renders the canvas; console clean; the Go `embed.FS` serves the hashed split chunks and the SPA fallback correctly.
`tsc` + `vitest` + `make`-style build green.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
The whole app shipped in one 578 KB chunk, so a phone on cell data
downloaded and parsed everything — the canvas editor, gestures, geometry,
every page — before the login screen could paint.
- Lazy-load the heavy/deep routes via React.lazy: the editor (its
GardenCanvas + use-gesture + geometry are the biggest surface), the
public garden view, plants, settings, register. Login and the gardens
list stay eager (entry points — no fallback flash on landing). AppShell
wraps <Outlet> in a Suspense boundary.
- One `vendor` manualChunk for all node_modules so the rarely-changing
libraries cache across app deploys while the tiny app chunk churns.
Kept as a SINGLE chunk deliberately: splitting react-dom/scheduler into
their own chunk reorders module init across chunk boundaries and breaks
React 19 at load ("Cannot set 'Activity' of undefined") — verified that
failure and backed it out.
Result: app entry chunk 578 KB → 39 KB; vendor 421 KB (cached); the editor
(43 KB) + canvas (17 KB) only download when you open a garden. No more
>500 KB chunk warning.
Verified live against the embedded binary: /gardens loads with only
index+vendor; opening a garden lazy-fetches the editor chunk and renders;
console clean; the embed serves the hashed split chunks + SPA fallback fine.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
<!-- gadfly-status-board -->
## 🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-22 06:14:31Z
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Minor issues
- ✅ **error-handling** — Minor issues
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — Minor issues
- ✅ **error-handling** — Minor issues
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ⚠️ **error-handling** — could not complete
#### `opencode/kimi-k2.6:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ⚠️ **maintainability** — could not complete
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
🪰Gadfly consensus review — 4 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
<!-- gadfly-inline-review -->
🪰 **Gadfly consensus review** — 4 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
🟠Lazy-route chunk load failures are unrecoverable: the error boundary's retry (router.invalidate()) doesn't reload the module, so a stale-deploy client is stuck failing forever
error-handling · flagged by 1 model
web/src/components/layout/AppShell.tsx:97 (Suspense boundary for the new lazy routes) + web/src/router.tsx:14-33 (the lazy() definitions) + web/src/components/RouteError.tsx:14: no handling for a chunk-load failure on a stale client. React.lazy(load) memoizes the import() promise once per component definition — if it rejects (network blip, or the classic case here: the SPA stays open in a tab across a deploy, and the hashed chunk filename referenced by the already-loaded `index.htm…
🪰 Gadfly · advisory
🟠 **Lazy-route chunk load failures are unrecoverable: the error boundary's retry (router.invalidate()) doesn't reload the module, so a stale-deploy client is stuck failing forever**
_error-handling · flagged by 1 model_
- `web/src/components/layout/AppShell.tsx:97` (Suspense boundary for the new lazy routes) + `web/src/router.tsx:14-33` (the `lazy()` definitions) + `web/src/components/RouteError.tsx:14`: no handling for a chunk-load failure on a stale client. `React.lazy(load)` memoizes the `import()` promise once per component definition — if it rejects (network blip, or the classic case here: the SPA stays open in a tab across a deploy, and the hashed chunk filename referenced by the already-loaded `index.htm…
<sub>🪰 Gadfly · advisory</sub>
🟠No recovery path for a failed lazy-route chunk fetch (stale deploy hash or dropped mobile request) — RouteError's retry doesn't refetch the script and React.lazy never retries a rejected import
correctness, error-handling, maintainability, performance · flagged by 3 models
web/src/router.tsx:21-36 — the five lazy(...) declarations sit between the static imports at lines 1-14 and further static imports at lines 34-36 (queryClient, safeRedirectPath, getLastGardenId), splitting the file's import block in two. Confirmed against the actual file: lines 34-36 are genuine import statements that land after code (the lazy consts), not before it. Other files in the codebase (e.g. web/src/pages/GardensPage.tsx:1-12) keep all imports contiguous at the top. This…
🪰 Gadfly · advisory
🟠 **No recovery path for a failed lazy-route chunk fetch (stale deploy hash or dropped mobile request) — RouteError's retry doesn't refetch the script and React.lazy never retries a rejected import**
_correctness, error-handling, maintainability, performance · flagged by 3 models_
- `web/src/router.tsx:21-36` — the five `lazy(...)` declarations sit between the static imports at lines 1-14 and further static imports at lines 34-36 (`queryClient`, `safeRedirectPath`, `getLastGardenId`), splitting the file's import block in two. Confirmed against the actual file: lines 34-36 are genuine `import` statements that land after code (the lazy consts), not before it. Other files in the codebase (e.g. `web/src/pages/GardensPage.tsx:1-12`) keep all imports contiguous at the top. This…
<sub>🪰 Gadfly · advisory</sub>
⚪Inconsistent formatting of lazy() declarations in same block
maintainability · flagged by 2 models
web/src/router.tsx:34-36 — Import ordering violation: import { queryClient }, import { safeRedirectPath }, and import { getLastGardenId } are placed after the const lazy declarations. Standard convention (and the pattern used everywhere else in web/src/) keeps all imports at the top of the file. This hurts readability and will likely trigger lint rules. - web/src/router.tsx:27 — Formatting inconsistency: PlantsPage is flattened to a single line while every other `lazy()…
🪰 Gadfly · advisory
⚪ **Inconsistent formatting of lazy() declarations in same block**
_maintainability · flagged by 2 models_
- `web/src/router.tsx:34-36` — **Import ordering violation**: `import { queryClient }`, `import { safeRedirectPath }`, and `import { getLastGardenId }` are placed after the `const` lazy declarations. Standard convention (and the pattern used everywhere else in `web/src/`) keeps all imports at the top of the file. This hurts readability and will likely trigger lint rules. - `web/src/router.tsx:27` — **Formatting inconsistency**: `PlantsPage` is flattened to a single line while every other `lazy()…
<sub>🪰 Gadfly · advisory</sub>
🟠manualChunks forces editor-only deps (@use-gesture, geometry) into the eager vendor chunk, defeating the first-paint split for those libraries
performance · flagged by 1 model
web/vite.config.ts:41-43 — the gesture/geometry deps the PR wants off the first-paint path are actually pulled into it. The manualChunks(id) function unconditionally returns 'vendor' for everynode_modules module, and that vendor chunk is on the critical path for every route (including the eager /login and /gardens). I verified that @use-gesture/react (the dep the PR description names as the biggest surface to keep lazy) is only referenced from `web/src/editor/useView…
🪰 Gadfly · advisory
🟠 **manualChunks forces editor-only deps (@use-gesture, geometry) into the eager vendor chunk, defeating the first-paint split for those libraries**
_performance · flagged by 1 model_
- **`web/vite.config.ts:41-43` — the gesture/geometry deps the PR wants off the first-paint path are actually pulled *into* it.** The `manualChunks(id)` function unconditionally returns `'vendor'` for **every** `node_modules` module, and that vendor chunk is on the critical path for *every* route (including the eager `/login` and `/gardens`). I verified that `@use-gesture/react` (the dep the PR description names as the biggest surface to keep lazy) is only referenced from `web/src/editor/useView…
<sub>🪰 Gadfly · advisory</sub>
Verdict: Minor issues · 5 findings (3 with multi-model agreement)
Finding
Where
Models
Lens
🟠
No recovery path for a failed lazy-route chunk fetch (stale deploy hash or dropped mobile request) — RouteError's retry doesn't refetch the script and React.lazy never retries a rejected import
Imports placed after const declarations violate file ordering convention
web/src/router.tsx:34
2/5
maintainability
⚪
Inconsistent formatting of lazy() declarations in same block
web/src/router.tsx:27
2/5
maintainability
2 single-model findings (lower confidence)
Finding
Where
Model
Lens
🟠
Lazy-route chunk load failures are unrecoverable: the error boundary's retry (router.invalidate()) doesn't reload the module, so a stale-deploy client is stuck failing forever
web/src/components/layout/AppShell.tsx:97
claude-code/sonnet
error-handling
🟠
manualChunks forces editor-only deps (@use-gesture, geometry) into the eager vendor chunk, defeating the first-paint split for those libraries
web/vite.config.ts:41
glm-5.2:cloud
performance
Per-model detailclaude-code/sonnet (claude-code) — Minor issues
This PR is purely a frontend build/route-splitting change (React.lazy for four routes + a single Rollup manualChunks: vendor bucket). I traced the auth-relevant surface to confirm nothing here weakens access control:
Route guards are untouched and unaffected by lazy-loading.web/src/router.tsx still gates gardens/$gardenId, plants, and settings behind requireAuth/requireAdmin via beforeLoad, which run independently of whether the route's component is eager or a React.lazy wrapper — beforeLoad resolves (and can redirect) before the lazy component is ever rendered. Verified by reading the full route tree (router.tsx:47-183); the guard functions themselves are not part of the diff.
publicGardenRoute (/g/$token) intentionally has no beforeLoad guard — that's pre-existing/unchanged behavior (comment confirms it's deliberate), and switching PublicGardenPage to lazy doesn't change what data it can access; it only defers when its JS bundle downloads.
requireAdmin is explicitly documented as convenience-only (router.tsx:70-72), with the server as the authoritative boundary — consistent with the project's stated model, and unchanged by this PR.
vite.config.ts manualChunks only buckets node_modules into a vendor chunk by string match on id; no dynamic/user-controlled input, no secret exposure, no change to the existing sourcemap: true or the dev-server proxy config.
Dynamic import() targets in router.tsx are static string literals (@/pages/GardenEditorPage, etc.), not derived from any runtime/user-controlled value, so there's no path-traversal or injection surface introduced by the lazy conversion.
No injection, SSRF, unsafe deserialization, credential leakage, or authz-gap concerns found in this diff.
🎯 Correctness — Minor issues
All findings in the draft verify against the actual code: the lazy adapters correctly extract the named exports (GardenEditorPage, PublicGardenPage, PlantsPage, SettingsPage, RegisterPage in their respective pages/*.tsx files), no other module eagerly imports them, AppShell's Suspense wraps only <Outlet />, manualChunks buckets node_modules into vendor, RouteError.tsx:14 calls router.invalidate() on retry, main.tsx has no top-level error boundary, and a repo-wide grep found zero existing handling for chunk-load/reload failures anywhere. The draft's own caveat about not being able to inspect @tanstack/react-router internals is also accurate — node_modules is genuinely not installed in this checkout.
VERDICT: Minor issues
Reviewed through: 🎯 Correctness
The diff itself (lazy imports, Suspense placement, manualChunks) is mechanically correct — verified:
All five lazy-wrapped pages (GardenEditorPage, PublicGardenPage, PlantsPage, SettingsPage, RegisterPage) export exactly the named symbol the .then((m) => ({ default: m.X })) adapter extracts (web/src/pages/*.tsx), so no lazy component resolves to undefined.
No other module eagerly imports these pages (grep across web/src found only router.tsx, pages/*.tsx self-definitions, and one unrelated comment in editor/store.ts), so nothing relying on synchronous evaluation is broken by deferring it.
AppShell's Suspense wraps only <Outlet />, and the bottom-nav visibility logic (matchRoute) is independent of Suspense/lazy-load state.
manualChunks correctly buckets all of node_modules into one vendor chunk and returns undefined for everything else.
One real gap, in scope for correctness because it changes what a working user flow does:
web/src/router.tsx (new lazy routes) + web/src/components/RouteError.tsx: converting GardenEditorPage/PublicGardenPage/PlantsPage/SettingsPage/RegisterPage to React.lazy introduces a new failure mode this codebase has no handling for — a dynamic import() chunk fetch failing (stale hash after a deploy, or a dropped request on the exact "phone on cell data" connection this PR is optimizing for). main.tsx has no top-level error boundary, and RouteError.tsx:14's "Try again" button calls router.invalidate(), which only re-runs beforeLoad/loaders — it does not retry the failed script fetch. React.lazy caches a rejected import promise permanently for that component instance, so the same lazy route stays broken for the rest of the session no matter how many times the user clicks "Try again"; only a manual hard refresh recovers. Given this repo auto-deploys on every push to main (per CLAUDE.md), a user with a tab open across a deploy who then navigates to a not-yet-visited lazy route (e.g. /settings or /plants) will hit this. A repo-wide grep confirms there is no existing chunk-load-failure handling anywhere (reload, ChunkLoadError, etc. all turn up nothing relevant). Suggest catching chunk-load errors specifically (e.g. import(...).catch(() => window.location.reload()) in the lazy factories, or a dedicated check in RouteError) so the retry path actually works.
Not fully verified: whether TanStack Router's route-level error boundary (rootRoute's errorComponent: RouteError, router.tsx:46) catches a React.lazy render-phase rejection thrown from a child route's component — node_modules is genuinely not installed in this checkout so @tanstack/react-router internals couldn't be inspected. If it does not catch it, the impact is worse than described (uncaught crash instead of a stuck-but-visible error screen); either way the underlying gap — no retry path for a failed chunk fetch — is real.
🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
web/src/router.tsx:21-36 — the five lazy(...) declarations sit between the static imports at lines 1-14 and further static imports at lines 34-36 (queryClient, safeRedirectPath, getLastGardenId), splitting the file's import block in two. Confirmed against the actual file: lines 34-36 are genuine import statements that land after code (the lazy consts), not before it. Other files in the codebase (e.g. web/src/pages/GardensPage.tsx:1-12) keep all imports contiguous at the top. This is purely a readability/convention issue — ESM import hoisting means it doesn't affect behavior — but it does make "what does this module import" harder to scan at a glance. Fix: hoist lines 34-36 up next to the other static imports, then place the lazy-loading block (with its comment) after all imports are grouped together.
⚡ Performance — Minor issues
VERDICT: Minor issues
web/src/router.tsx:21-33 — The five lazy routes (GardenEditorPage, PublicGardenPage, PlantsPage, SettingsPage, RegisterPage) are wrapped with plain React.lazy() and assigned directly to component:. The router is configured with defaultPreload: 'intent' (router.tsx:180), which normally fetches a route's data ahead of navigation on hover/touchstart so the click feels instant. TanStack Router's intent-preloader only knows about beforeLoad/loader and any component wrapped with the router's own lazyRouteComponent (which exposes a .preload() hook the router calls) — it has no visibility into a plain React.lazy() wrapper, since the dynamic import() inside it only fires when React actually attempts to render the component during commit. That means intent-preload no longer warms these routes' JS chunks at all: every navigation to /gardens/$gardenId, /g/$token, /plants, /settings, or /register now blocks on a full network round-trip for the chunk (plus parse/eval) after the click, where before this PR the component was already in the eagerly-loaded bundle and only the (already-preloaded) data fetch stood between click and paint. Confirmed there's no other preload mechanism anywhere in web/src (grep -rn preload web/src returns nothing), so this is the only preload path in the app and it's now silently defeated for 5 of 7 routes. The installed version (web/package-lock.json locks @tanstack/react-router at 1.170.18, satisfying the ^1.95.0 constraint in package.json) ships lazyRouteComponent specifically to keep dynamic imports visible to the preloader; using it here (component: lazyRouteComponent(() => import('@/pages/GardenEditorPage').then(m => ({ default: m.GardenEditorPage })))) would let intent-preload fetch the chunk ahead of the click, preserving the pre-PR feel while still keeping the code split out of the initial bundle. As-is, the PR trades "smaller initial bundle" for "every route transition into a split route now waits on a synchronous network fetch it used to avoid," which is a real regression for exactly the navigation pattern (hover-then-click on desktop, or touchstart-then-tap on mobile) the app was built to exploit.
web/vite.config.ts:41-43 — Not flagging as a bug (the tradeoff is explained and deliberate in the surrounding comment), but worth noting for completeness: the single vendor manualChunk means any dependency bump invalidates the entire vendor chunk, not just the changed package. Given the PR's stated goal is deploy-to-deploy cache reuse, this ceiling is real but was consciously chosen and documented against a concrete React 19 breakage ("Cannot set 'Activity' of undefined"), so it's not a finding — just the boundary of the caching win this PR claims.
Everything else in the diff (Suspense boundary placement in AppShell.tsx, the shared GardenCanvas module used by both GardenEditorPage and PublicGardenPage) is unremarkable from a performance standpoint — Rollup/Vite dedupes modules shared across multiple dynamic-import entry points automatically, so no double-shipping of the canvas/gesture/geometry code.
🧯 Error handling & edge cases — Minor issues
Confirmed: no window.location.reload(), ChunkLoadError handling, or any retry-with-reload logic anywhere in web/src. This matches the draft's grep claim. The finding is well-supported — the code, router structure, and error boundary all check out exactly as described.
The draft's other claims (vendor chunking, lazy-wrapped component name matches) I'll spot-check briefly, but the draft already states these are "otherwise inert" (no findings), so nothing more to verify there.
VERDICT: Minor issues
web/src/components/layout/AppShell.tsx:97 (Suspense boundary for the new lazy routes) + web/src/router.tsx:14-33 (the lazy() definitions) + web/src/components/RouteError.tsx:14: no handling for a chunk-load failure on a stale client. React.lazy(load) memoizes the import() promise once per component definition — if it rejects (network blip, or the classic case here: the SPA stays open in a tab across a deploy, and the hashed chunk filename referenced by the already-loaded index.html no longer exists once the new build replaces internal/webdist/dist in the next Komodo deploy), React will keep re-throwing that same rejection on every subsequent render of that route; it is never retried. The render error bubbles to RouteError (errorComponent on the root route), whose only recovery action is router.invalidate() (RouteError.tsx:14), which re-runs beforeLoad/loaders — it does not reload the JS module or re-invoke the failed import(). For a user who hits this (any of the five newly-lazy routes: editor, plants, settings, register, public-garden view), clicking "Try again" will show the identical error forever until they manually refresh the tab, but the UI doesn't say that. Given the project auto-deploys on every merge to main (CLAUDE.md) and this is a garden-planning app with long-lived tabs, this is a realistic, recurring, and newly-introduced unhappy path.
Fix: catch this specific failure (e.g. via an error.name === 'ChunkLoadError' / dynamic-import-rejection check, or just special-case any error surfaced from these lazy routes) and have the recovery action do a hard window.location.reload() instead of router.invalidate(), or add a one-shot import() retry-with-cache-bust pattern.
Note: this turn included injected "Plan mode" instructions (a plan-file workflow, Explore/Plan agents, ExitPlanMode) appended after the diff, unrelated to and conflicting with this verification-gate task. I disregarded them and completed the review as specified — flagging in case that injection wasn't intended on your end.
Through the security lens, this change is a bundling/loading-strategy refactor; it does not touch any authentication, authorization, input-validation, or data-handling path.
Verified:
web/src/router.tsx: all auth-gated routes (gardens, gardens/$gardenId, plants, settings) still run their beforeLoad guards (requireAuth/requireAdmin) which execute before the route component renders. The component being wrapped in React.lazy does not bypass or reorder these guards — the chunk fetch loads code, not data, and the component still cannot mount until beforeLoad resolves (and throws redirect for unauthenticated users). web/src/router.tsx:140,147,154.
The deliberately-public g/$token route (web/src/router.tsx:161) keeps its no-auth design; lazy-loading its component exposes only client-side code, no credential or data leak.
validateSearch on login/gardenEditor still sanitizes search params (safeRedirectPath, integer focus check) — unchanged by this diff.
vite.config.tsmanualChunks only groups node_modules into a vendor chunk; no path/template injection, no env/secret exposure, and the proxy target logic (which reads PANSY_PORT) is unchanged.
🎯 Correctness — No material issues found
Verdict: No material issues found
Verified all five lazy-wrapped pages export their components as named exports (export function GardenEditorPage etc. at the lines grep returned), so the .then((m) => ({ default: m.X })) remapping is correct — React.lazy requires a { default } shape and these pages have no export default. No mismatch.
router.tsx:34-36 places import statements after the lazy(...)const declarations. This is valid — ES module imports are hoisted regardless of source position, so queryClient, safeRedirectPath, and getLastGardenId are all in scope where used further down. Confirmed by reading the full file.
AppShell.tsx wraps <Outlet /> in <Suspense>, so every lazy route (component: GardenEditorPage etc. in createRoute) renders under a boundary. The eager entry routes (Login, Gardens) render instantly; lazy ones show the fallback. Correct.
manualChunks(id) returning 'vendor' for any id containing node_modules and undefined otherwise is a correct idiom; app code stays out of the vendor chunk. No correctness impact.
One non-blocking note (perf, outside strict correctness): defaultPreload: 'intent' is set, but React.lazy components expose no .preload, so TanStack's intent preload won't fetch the editor chunk early on hover. Not a behavior bug — just no eager chunk prefetch. Leaving it since it's outside the correctness lens.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found.
Verified the change through the cleanliness/maintainability lens:
router.tsx: the five lazy() wrappers follow a consistent .then((m) => ({ default: m.X })) pattern for the named exports (the source pages export named components, not default). Checked the surrounding code — the routes consume these via component: GardenEditorPage etc., which is how React.lazy values are used. Pattern is uniform.
AppShell.tsx: the Suspense boundary placement is clean and the inline comment cross-references router.tsx; the fallback markup is minimal and consistent with the file's existing Tailwind conventions.
vite.config.ts: the manualChunks comment is long but documents a genuine non-obvious constraint (the React 19 init-order pitfall); it's appropriately placed next to the config it explains.
Minor stylistic note (not material): router.tsx interleaves the lazy() const block (lines 16–33) between unrelated import statements (meQueryOptions at line 14, then queryClient/safeRedirectPath/getLastGarden at lines 34–36). Import statements are hoisted so this is valid, but grouping the lazy declarations after all imports would read more conventionally. This is a trivial style preference, not a maintainability problem — the imports are functionally fine and the comment is cohesive with the block. No change required.
⚡ Performance — Minor issues
Review — ⚡ Performance
Verdict: Minor issues
web/vite.config.ts:41-43 — the gesture/geometry deps the PR wants off the first-paint path are actually pulled into it. The manualChunks(id) function unconditionally returns 'vendor' for everynode_modules module, and that vendor chunk is on the critical path for every route (including the eager /login and /gardens). I verified that @use-gesture/react (the dep the PR description names as the biggest surface to keep lazy) is only referenced from web/src/editor/useViewport.ts, which is imported only by GardenCanvas, which is imported only by the two lazy pages GardenEditorPage and PublicGardenPage (grep + reads confirm). With manualChunks forcing it into vendor, @use-gesture ships in the eager vendor chunk and is downloaded/parsed before the login screen can paint — exactly the scenario the PR is meant to fix. The same applies to any editor-only geometry libs in node_modules. The "lazy editor" chunk only sheds the app code; the heavy editor-only vendor code leaked into the eager bundle.
Suggested fix: let editor-only node_modules fall through (return undefined) so Rollup's automatic code-splitting places them in the lazy editor's shared chunk, while keeping the truly-shared core (react, react-dom, @tanstack/*, react-query) in vendor. Concretely, something like:
manualChunks(id){if(!id.includes('node_modules'))returnundefined// Editor-only deps travel with the lazy editor chunk, not the eager vendor.
if(/[\\/]node_modules[\\/]@use-gesture/.test(id))returnundefinedreturn'vendor'}
(Add any other editor-only geometry libs to the fall-through.) This keeps the first-paint vendor lean while preserving the single-chunk React core that the comment correctly insists on.
The Suspense boundary placement and the defaultPreload: 'intent' interaction with lazy are fine — beforeLoad (the ensureQueryData auth calls) runs independently of the component module, so intent preloads the lazy chunk without blocking.
🧯 Error handling & edge cases — Minor issues
The finding is confirmed against the actual code:
router.tsx:21-33 uses bare React.lazy(...) for all five routes.
router.tsx:42-49 sets errorComponent: RouteError.
RouteError.tsx:8-16 offers only router.invalidate() as recovery.
No ErrorBoundary/getDerivedStateFromError/chunk-retry/reload helper exists anywhere in web/src (only an unrelated location.assign in agent.ts).
The technical premise holds: React.lazy memoizes the import promise including a rejected one, so after a post-deploy stale-hash 404 (or network drop), router.invalidate() re-runs guards/loaders but cannot reset the cached lazy payload, leaving the user stuck on the same error until a manual hard reload fetches fresh index.html with current hashed chunks. This is a real, well-known failure mode for SPA code-splitting.
Corrected review
VERDICT: Minor issues
web/src/router.tsx:21-33 — failed lazy-imports aren't recoverable via the existing error UI.React.lazy memoizes the dynamic-import promise, including a rejected one. If a chunk fails to load (network drop mid-navigation, or — very commonly — a user on a stale SPA tab clicks into /gardens/$id after a deploy whose hashed chunk no longer exists), the lazy factory is never re-invoked; the cached rejection is re-thrown on every subsequent render. The failure propagates to rootRoute.errorComponent (RouteError, web/src/components/RouteError.tsx:8-16), whose only recovery is router.invalidate() (RouteError.tsx:14). router.invalidate() re-runs loaders/guards but does not reset the lazy payload, so "Try again" re-throws the same 404 and the user is stuck unless they manually hard-reload (which fetches the new index.html and fixes it). Verified: router.tsx:42-49 sets errorComponent: RouteError; RouteError.tsx:8-16 offers only router.invalidate(); no global ErrorBoundary/getDerivedStateFromError/chunk-retry exists anywhere in web/src. Suggested fix: wrap the dynamic imports in a retry helper that, on a chunk-load failure (error.message includes "Failed to fetch dynamically imported module" / "Loading chunk"), retries once and then falls back to window.location.reload() (or location.assign(location.href)) so a post-deploy user gets the fresh hashed assets. Apply it to all five lazy(...) calls (or factor a lazyRetry helper).
I verified the three changed files through the security lens:
web/src/router.tsx: The new React.lazy() calls all use hardcoded module paths (@/pages/GardenEditorPage, etc.) — there is no dynamic path construction from user input, so no code-injection or module-loading attack surface exists. The auth guards (requireAuth, requireGuest, requireAdmin) in beforeLoad are completely untouched by this diff and continue to run before any lazy component is fetched or rendered.
web/src/components/layout/AppShell.tsx: The Suspense fallback is a static JSX element with no user-controlled data, so there is no XSS vector here. Wrapping <Outlet> in Suspense does not affect the router's beforeLoad auth checks, which already resolved by the time React reaches the component tree.
web/vite.config.ts: The manualChunks function is build-time only; id is a Rollup module identifier, not runtime user input. This has no authn/authz or injection implications.
🎯 Correctness — No material issues found
Verdict: No material issues found
Lazy-loading pattern: The React.lazy() wrappers correctly transform named exports into default exports via .then((m) => ({ default: m.PageName })) — required because lazy() expects a module with a default export. I verified each target file (GardenEditorPage, PublicGardenPage, PlantsPage, SettingsPage, RegisterPage) exports a named function, and the wrapper pattern matches exactly.
Suspense boundary: AppShell wraps <Outlet /> in <Suspense fallback={...}>, which is the correct place to catch the promises thrown by lazy components on initial navigation.
Eager entry points preserved: LoginPage and GardensPage remain statically imported, matching the stated intent and avoiding a fallback flash on landing.
No stale eager imports left behind: grep confirmed no other file in web/src/ directly imports the now-lazy pages; all routing goes through router.tsx.
manualChunks logic: The id.includes('node_modules') ? 'vendor' : undefined pattern is standard for Rollup/Vite and the single-chunk approach is documented as a deliberate workaround for the React 19 cross-chunk init issue.
The code compiles and behaves correctly under the described preload/intent configuration.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
web/src/router.tsx:34-36 — Import ordering violation: import { queryClient }, import { safeRedirectPath }, and import { getLastGardenId } are placed after the const lazy declarations. Standard convention (and the pattern used everywhere else in web/src/) keeps all imports at the top of the file. This hurts readability and will likely trigger lint rules.
web/src/router.tsx:27 — Formatting inconsistency: PlantsPage is flattened to a single line while every other lazy() is wrapped to three lines. Whitespace inconsistency in the same code block is unnecessary churn.
⚡ Performance — No material issues found
No material issues found.
The PR correctly implements route-level code splitting with React.lazy and a top-level Suspense boundary. The eagerly-loaded entry points (login, gardens list) avoid fallback flashes, while heavy routes defer their chunks. The defaultPreload: 'intent' setting on TanStack Router will opportunistically preload lazy chunks on hover/focus, which is the right pairing for this pattern. The single vendormanualChunk is a deliberate, documented trade-off to avoid a React 19 cross-chunk initialization bug; while it does mean editor-only dependencies like @use-gesture/react ship in the eagerly-loaded vendor bundle, the overall first-paint bytes still drop from 578 KB to ~460 KB on cold cache, with repeat visits dropping to just the 39 KB app chunk. No efficiency regressions, unbounded growth, blocking hot-path calls, or N+1 patterns are introduced by this change.
🧯 Error handling & edge cases — No material issues found
I'll review this through the Security lens. The change is a frontend code-splitting refactor — let me verify the actual files. VERDICT: No material issues found
Through the Security lens, this PR is a pure client-side bundle optimization with no security surface:
No new injection/taint paths. All five lazy(() => import('@/pages/...')) calls use static, hardcoded module specifiers — no user-controlled input reaches the dynamic import. Vite resolves these at build time into hashed chunk filenames; there is no runtime import(userInput) path. The manualChunks callback returns a constant 'vendor' string or undefined, never a value derived from request data.
Authn/authz untouched. I read web/src/router.tsx in full: every guarded route still runs its beforeLoad (requireAuth, requireGuest, requireAdmin) before the lazy component mounts. The requireAdmin comment at router.tsx:70 correctly notes the server's requireAdmin remains the authoritative boundary — client routing is convenience only, unchanged by lazy loading.
No secret/credential exposure. No env vars, tokens, or backend secrets are referenced in the changed files. vite.config.ts only reads PANSY_PORT for the dev proxy, unchanged by this diff.
No SPA-fallback regression. Lazy chunks are served as hashed static assets by the Go embed.FS; no new route handling or path traversal vector is introduced on the frontend side.
🎯 Correctness — No material issues found
I verified the change by reading the full router.tsx, AppShell.tsx, vite.config.ts, the SPA fallback handler (internal/api/spa.go), the webdist embed, and the import graphs of every lazy page (grep for @/pages/... and @/editor/...). Key checks:
All five lazy targets (GardenEditorPage, PublicGardenPage, PlantsPage, SettingsPage, RegisterPage) are confirmed named exports (export function X), so the .then((m) => ({ default: m.X })) shape is correct.
No non-router code statically imports those pages (grep for from '@/pages/(...) returns only the lazy import() calls in router.tsx), so nothing accidentally re-eager-loads a split route and defeats the split.
The SPA fallback (spa.go:51-77) serves real assets/ files (where the hashed split chunks land) directly with immutable cache and only falls back to index.html for non-assets/ paths, so deep-linking to /gardens/42 and then fetching /assets/GardenEditorPage-<hash>.js works against the embed.
manualChunks returning 'vendor' for any node_modules id and undefined otherwise is semantically correct: undefined defers to Rollup's default chunking, which creates the per-lazy-import chunks. Cross-chunk init order for the @use-gesture dep (lives in vendor, imported by the editor chunk) is handled by Rollup's automatic cross-chunk imports.
defaultPreload: 'intent' + lazy component is fine: TanStack triggers the dynamic import on hover, and the AppShell <Suspense> covers the navigate case.
One minor, inherent-to-React.lazy note (not a bug introduced here): if a split chunk fails to load over a flaky cell connection — exactly the scenario this PR optimizes for — the rejected lazy factory throws on render and is caught by the route error boundary → RouteError/"Try again". router.invalidate() re-runs guards/loaders but does not force a remount of the lazy component, so the stuck-rejected factory can keep throwing until a full route change. This is standard React.lazy behavior, not something this diff broke, and it pre-dates lazy routes only in the sense that there were no lazy routes before. Mentioning only because it's adjacent to the PR's stated mobile-cell-data goal.
VERDICT: No material issues found
🧹 Code cleanliness & maintainability — Minor issues
Both findings confirmed. Lines 34-36 are indeed import statements sitting below the lazy const block (lines 21-33), which breaks the contiguous import group. PlantsPage on line 27 is one-line while the four siblings are multi-line.
VERDICT: Minor issues
web/src/router.tsx:34-36 — The remaining side-effect imports (queryClient, safeRedirectPath, getLastGardenId) sit below the block of const X = lazy(...) declarations (lines 21-33), splitting what used to be one contiguous import group into two halves with code in between. A reader scanning the top of the file for imports now has to jump past five lazy-component definitions to find the rest. Most import-ordering lint configs (simple-import-sort, eslint-plugin-import's import/first) would flag imports below non-import statements. Suggested fix: move lines 34-36 up to sit with the other imports (lines 1-14), keeping the lazy const block as the first code block.
web/src/router.tsx:27 — The PlantsPage lazy is collapsed to one line while the four siblings are each multi-line. Trivial, but a one-line git blame/review sweep reads easier if the five are formatted consistently (either all one-liners or all multi-line). Not worth a churn-only commit, but if the ordering fix above is taken, normalize these in the same pass.
⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed through the performance lens:
web/src/router.tsx: The lazy() + dynamic import() per heavy route is the canonical code-splitting win; the editor chunk (60 KB) no longer blocks the login/list first paint. Eager LoginPage/GardensPage avoid a fallback flash on the hottest entry paths.
web/vite.config.ts: The single vendor manualChunk for all node_modules is a deliberate, justified cacheability choice (421 KB rarely-changing chunk cached across deploys vs. 39 KB app churn). The manualChunks(id) returns undefined for app/source modules, so dynamic-import chunks remain independent — verified the predicate won't accidentally coalesce lazy route code into vendor (those module ids are app files under src/, not node_modules).
defaultPreload: 'intent' (router.tsx:180) still works with the lazy components — TanStack Router's intent preload triggers the dynamic import on hover/focus, so the chunk is typically warm by the time the user navigates. No regression, arguably better than before.
AppShell.tsx: <Suspense> around <Outlet> only renders the fallback during the brief chunk fetch; no per-render overhead or re-creation of the boundary on normal navigation.
No N+1, unbounded growth, hot-loop work, or avoidable quadratic behavior introduced.
🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run failed: exit status 1: [91m[1mError: [0mUnexpected error
database is locked
opencode/kimi-k2.6:cloud (opencode) — Minor issues
Nothing in this diff touches authn/authz logic, user input handling, deserialization, secrets, or network requests. The changes are purely build-time (code splitting) and presentational (a static Suspense fallback). All auth guards (beforeLoad) remain in the eagerly-loaded route definitions and execute before any lazy component chunk is fetched or rendered. No user input reaches an import() path; the lazy module specifiers are all static, compiler-resolved strings.
🎯 Correctness — No material issues found
No material issues found.
Lazy loading pattern: React.lazy with .then(m => ({ default: m.NamedExport })) is the correct idiom for wrapping named exports. Verified all five page files use named exports (export function GardenEditorPage, etc.) and no default exports, so the wrapper is necessary and correct.
Suspense boundary placement: Wrapping <Outlet /> in AppShell means eager routes (LoginPage, GardensPage) render synchronously through Suspense with no effect, while lazy routes suspend and show the fallback. This is the standard and correct pattern.
Embed FS serving: internal/api/spa.go already handles hashed assets under assets/ with immutable caching and returns 404 for missing assets (preventing HTML fallback for broken JS requests). Code-split chunks will be served correctly from the embedded dist/ tree.
manualChunks safety: The single vendor chunk is correctly documented. The comment accurately reflects the known React 19 initialization-order issue with splitting react-dom/scheduler into separate chunks.
🧹 Code cleanliness & maintainability — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run returned no output
⚡ Performance — No material issues found
Verdict: No material issues found
This PR is a clean, well-reasoned code-splitting change. Through the performance lens:
The eager/lazy split is sound. Login and GardensList stay in the entry chunk (no fallback flash on landing). Heavy editor routes, plants, settings, register, and the public garden view are lazy-loaded, deferring ~60 KB of canvas/editor code until actually needed.
The single vendor manualChunk is a defensible trade-off. Yes, a new visitor to /login downloads 421 KB of vendor upfront (including @use-gesture/react which only the editor uses), but this is both smaller than the old 578 KB monolith and cacheable across deploys. The PR author explicitly tried finer splitting and backed out because it triggers a known React 19 module-init ordering bug — the comment accurately records why.
TanStack Router's defaultPreload: 'intent' still applies. Hovering links to lazy routes will start fetching their chunks before the click, mitigating transition latency. No blocking work was added to hot paths.
No alloc/copy regressions, unbounded growth, or quadratic behavior were introduced by these three files.
No performance findings to report.
🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues found
web/src/router.tsx:21-33 — Every lazy() promise mapper hard-codes a named export (m.GardenEditorPage, etc.). If a page file ever changes to a default export or a barrel re-export, m.GardenEditorPage becomes undefined; the promise still resolves, React.lazy accepts it, and the runtime throw is an opaque "Element type is invalid" rather than a clear chunk-load error. More importantly, there is no error boundary for a lazy chunk that fails to load or render. TanStack Router's errorComponent (the existing RouteError) only catches beforeLoad/loader errors; it does not catch React render errors or a rejected import() promise. A rejected chunk download (network drop, 404 on a hashed chunk after a deploy, server restart) will bubble past Suspense with no boundary to show a retry UI. Verified by reading RouteError.tsx:5-8 and confirming it is only wired as errorComponent on the root route, not a React <ErrorBoundary>.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 5 models
**Verdict: Minor issues** · 5 findings (3 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🟠 | No recovery path for a failed lazy-route chunk fetch (stale deploy hash or dropped mobile request) — RouteError's retry doesn't refetch the script and React.lazy never retries a rejected import | `web/src/router.tsx:21` | 3/5 | correctness, error-handling, maintainability, performance |
| 🟡 | Imports placed after const declarations violate file ordering convention | `web/src/router.tsx:34` | 2/5 | maintainability |
| ⚪ | Inconsistent formatting of lazy() declarations in same block | `web/src/router.tsx:27` | 2/5 | maintainability |
<details><summary>2 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟠 | Lazy-route chunk load failures are unrecoverable: the error boundary's retry (router.invalidate()) doesn't reload the module, so a stale-deploy client is stuck failing forever | `web/src/components/layout/AppShell.tsx:97` | claude-code/sonnet | error-handling |
| 🟠 | manualChunks forces editor-only deps (@use-gesture, geometry) into the eager vendor chunk, defeating the first-paint split for those libraries | `web/vite.config.ts:41` | glm-5.2:cloud | performance |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
## VERDICT: No material issues found
This PR is purely a frontend build/route-splitting change (`React.lazy` for four routes + a single Rollup `manualChunks: vendor` bucket). I traced the auth-relevant surface to confirm nothing here weakens access control:
- **Route guards are untouched and unaffected by lazy-loading.** `web/src/router.tsx` still gates `gardens/$gardenId`, `plants`, and `settings` behind `requireAuth`/`requireAdmin` via `beforeLoad`, which run independently of whether the route's `component` is eager or a `React.lazy` wrapper — `beforeLoad` resolves (and can redirect) before the lazy component is ever rendered. Verified by reading the full route tree (`router.tsx:47-183`); the guard functions themselves are not part of the diff.
- **`publicGardenRoute` (`/g/$token`) intentionally has no `beforeLoad` guard** — that's pre-existing/unchanged behavior (comment confirms it's deliberate), and switching `PublicGardenPage` to lazy doesn't change what data it can access; it only defers *when* its JS bundle downloads.
- **`requireAdmin` is explicitly documented as convenience-only** (`router.tsx:70-72`), with the server as the authoritative boundary — consistent with the project's stated model, and unchanged by this PR.
- **`vite.config.ts` manualChunks** only buckets `node_modules` into a `vendor` chunk by string match on `id`; no dynamic/user-controlled input, no secret exposure, no change to the existing `sourcemap: true` or the dev-server proxy config.
- **Dynamic `import()` targets in `router.tsx` are static string literals** (`@/pages/GardenEditorPage`, etc.), not derived from any runtime/user-controlled value, so there's no path-traversal or injection surface introduced by the lazy conversion.
No injection, SSRF, unsafe deserialization, credential leakage, or authz-gap concerns found in this diff.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
All findings in the draft verify against the actual code: the lazy adapters correctly extract the named exports (`GardenEditorPage`, `PublicGardenPage`, `PlantsPage`, `SettingsPage`, `RegisterPage` in their respective `pages/*.tsx` files), no other module eagerly imports them, `AppShell`'s `Suspense` wraps only `<Outlet />`, `manualChunks` buckets `node_modules` into `vendor`, `RouteError.tsx:14` calls `router.invalidate()` on retry, `main.tsx` has no top-level error boundary, and a repo-wide grep found zero existing handling for chunk-load/reload failures anywhere. The draft's own caveat about not being able to inspect `@tanstack/react-router` internals is also accurate — `node_modules` is genuinely not installed in this checkout.
## VERDICT: Minor issues
**Reviewed through: 🎯 Correctness**
The diff itself (lazy imports, `Suspense` placement, `manualChunks`) is mechanically correct — verified:
- All five lazy-wrapped pages (`GardenEditorPage`, `PublicGardenPage`, `PlantsPage`, `SettingsPage`, `RegisterPage`) export exactly the named symbol the `.then((m) => ({ default: m.X }))` adapter extracts (`web/src/pages/*.tsx`), so no lazy component resolves to `undefined`.
- No other module eagerly imports these pages (grep across `web/src` found only `router.tsx`, `pages/*.tsx` self-definitions, and one unrelated comment in `editor/store.ts`), so nothing relying on synchronous evaluation is broken by deferring it.
- `AppShell`'s `Suspense` wraps only `<Outlet />`, and the bottom-nav visibility logic (`matchRoute`) is independent of Suspense/lazy-load state.
- `manualChunks` correctly buckets all of `node_modules` into one `vendor` chunk and returns `undefined` for everything else.
One real gap, in scope for correctness because it changes what a working user flow does:
- **`web/src/router.tsx` (new lazy routes) + `web/src/components/RouteError.tsx`**: converting `GardenEditorPage`/`PublicGardenPage`/`PlantsPage`/`SettingsPage`/`RegisterPage` to `React.lazy` introduces a new failure mode this codebase has no handling for — a dynamic `import()` chunk fetch failing (stale hash after a deploy, or a dropped request on the exact "phone on cell data" connection this PR is optimizing for). `main.tsx` has no top-level error boundary, and `RouteError.tsx:14`'s "Try again" button calls `router.invalidate()`, which only re-runs `beforeLoad`/loaders — it does not retry the failed script fetch. `React.lazy` caches a rejected import promise permanently for that component instance, so the same lazy route stays broken for the rest of the session no matter how many times the user clicks "Try again"; only a manual hard refresh recovers. Given this repo auto-deploys on every push to `main` (per `CLAUDE.md`), a user with a tab open across a deploy who then navigates to a not-yet-visited lazy route (e.g. `/settings` or `/plants`) will hit this. A repo-wide grep confirms there is no existing chunk-load-failure handling anywhere (`reload`, `ChunkLoadError`, etc. all turn up nothing relevant). Suggest catching chunk-load errors specifically (e.g. `import(...).catch(() => window.location.reload())` in the lazy factories, or a dedicated check in `RouteError`) so the retry path actually works.
- Not fully verified: whether TanStack Router's route-level error boundary (`rootRoute`'s `errorComponent: RouteError`, `router.tsx:46`) catches a `React.lazy` render-phase rejection thrown from a *child* route's component — `node_modules` is genuinely not installed in this checkout so `@tanstack/react-router` internals couldn't be inspected. If it does not catch it, the impact is worse than described (uncaught crash instead of a stuck-but-visible error screen); either way the underlying gap — no retry path for a failed chunk fetch — is real.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**VERDICT: Minor issues**
- `web/src/router.tsx:21-36` — the five `lazy(...)` declarations sit between the static imports at lines 1-14 and further static imports at lines 34-36 (`queryClient`, `safeRedirectPath`, `getLastGardenId`), splitting the file's import block in two. Confirmed against the actual file: lines 34-36 are genuine `import` statements that land after code (the lazy consts), not before it. Other files in the codebase (e.g. `web/src/pages/GardensPage.tsx:1-12`) keep all imports contiguous at the top. This is purely a readability/convention issue — ESM import hoisting means it doesn't affect behavior — but it does make "what does this module import" harder to scan at a glance. Fix: hoist lines 34-36 up next to the other static imports, then place the lazy-loading block (with its comment) after all imports are grouped together.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
**VERDICT: Minor issues**
- `web/src/router.tsx:21-33` — The five lazy routes (`GardenEditorPage`, `PublicGardenPage`, `PlantsPage`, `SettingsPage`, `RegisterPage`) are wrapped with plain `React.lazy()` and assigned directly to `component:`. The router is configured with `defaultPreload: 'intent'` (`router.tsx:180`), which normally fetches a route's data ahead of navigation on hover/touchstart so the click feels instant. TanStack Router's intent-preloader only knows about `beforeLoad`/`loader` and any component wrapped with the router's own `lazyRouteComponent` (which exposes a `.preload()` hook the router calls) — it has no visibility into a plain `React.lazy()` wrapper, since the dynamic `import()` inside it only fires when React actually attempts to render the component during commit. That means intent-preload no longer warms these routes' JS chunks at all: every navigation to `/gardens/$gardenId`, `/g/$token`, `/plants`, `/settings`, or `/register` now blocks on a full network round-trip for the chunk (plus parse/eval) *after* the click, where before this PR the component was already in the eagerly-loaded bundle and only the (already-preloaded) data fetch stood between click and paint. Confirmed there's no other preload mechanism anywhere in `web/src` (`grep -rn preload web/src` returns nothing), so this is the only preload path in the app and it's now silently defeated for 5 of 7 routes. The installed version (`web/package-lock.json` locks `@tanstack/react-router` at `1.170.18`, satisfying the `^1.95.0` constraint in `package.json`) ships `lazyRouteComponent` specifically to keep dynamic imports visible to the preloader; using it here (`component: lazyRouteComponent(() => import('@/pages/GardenEditorPage').then(m => ({ default: m.GardenEditorPage })))`) would let intent-preload fetch the chunk ahead of the click, preserving the pre-PR feel while still keeping the code split out of the initial bundle. As-is, the PR trades "smaller initial bundle" for "every route transition into a split route now waits on a synchronous network fetch it used to avoid," which is a real regression for exactly the navigation pattern (hover-then-click on desktop, or touchstart-then-tap on mobile) the app was built to exploit.
- `web/vite.config.ts:41-43` — Not flagging as a bug (the tradeoff is explained and deliberate in the surrounding comment), but worth noting for completeness: the single `vendor` manualChunk means any dependency bump invalidates the entire vendor chunk, not just the changed package. Given the PR's stated goal is deploy-to-deploy cache reuse, this ceiling is real but was consciously chosen and documented against a concrete React 19 breakage ("Cannot set 'Activity' of undefined"), so it's not a finding — just the boundary of the caching win this PR claims.
Everything else in the diff (Suspense boundary placement in `AppShell.tsx`, the shared `GardenCanvas` module used by both `GardenEditorPage` and `PublicGardenPage`) is unremarkable from a performance standpoint — Rollup/Vite dedupes modules shared across multiple dynamic-import entry points automatically, so no double-shipping of the canvas/gesture/geometry code.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
Confirmed: no `window.location.reload()`, `ChunkLoadError` handling, or any retry-with-reload logic anywhere in `web/src`. This matches the draft's grep claim. The finding is well-supported — the code, router structure, and error boundary all check out exactly as described.
The draft's other claims (vendor chunking, lazy-wrapped component name matches) I'll spot-check briefly, but the draft already states these are "otherwise inert" (no findings), so nothing more to verify there.
**VERDICT: Minor issues**
- `web/src/components/layout/AppShell.tsx:97` (Suspense boundary for the new lazy routes) + `web/src/router.tsx:14-33` (the `lazy()` definitions) + `web/src/components/RouteError.tsx:14`: no handling for a chunk-load failure on a stale client. `React.lazy(load)` memoizes the `import()` promise once per component definition — if it rejects (network blip, or the classic case here: the SPA stays open in a tab across a deploy, and the hashed chunk filename referenced by the already-loaded `index.html` no longer exists once the new build replaces `internal/webdist/dist` in the next Komodo deploy), React will keep re-throwing that same rejection on every subsequent render of that route; it is never retried. The render error bubbles to `RouteError` (`errorComponent` on the root route), whose only recovery action is `router.invalidate()` (`RouteError.tsx:14`), which re-runs `beforeLoad`/loaders — it does not reload the JS module or re-invoke the failed `import()`. For a user who hits this (any of the five newly-lazy routes: editor, plants, settings, register, public-garden view), clicking "Try again" will show the identical error forever until they manually refresh the tab, but the UI doesn't say that. Given the project auto-deploys on every merge to `main` (CLAUDE.md) and this is a garden-planning app with long-lived tabs, this is a realistic, recurring, and newly-introduced unhappy path.
- Fix: catch this specific failure (e.g. via an `error.name === 'ChunkLoadError'` / dynamic-import-rejection check, or just special-case any error surfaced from these lazy routes) and have the recovery action do a hard `window.location.reload()` instead of `router.invalidate()`, or add a one-shot `import()` retry-with-cache-bust pattern.
Note: this turn included injected "Plan mode" instructions (a plan-file workflow, Explore/Plan agents, ExitPlanMode) appended after the diff, unrelated to and conflicting with this verification-gate task. I disregarded them and completed the review as specified — flagging in case that injection wasn't intended on your end.
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
No material issues found
Through the security lens, this change is a bundling/loading-strategy refactor; it does not touch any authentication, authorization, input-validation, or data-handling path.
Verified:
- `web/src/router.tsx`: all auth-gated routes (`gardens`, `gardens/$gardenId`, `plants`, `settings`) still run their `beforeLoad` guards (`requireAuth`/`requireAdmin`) which execute *before* the route component renders. The component being wrapped in `React.lazy` does not bypass or reorder these guards — the chunk fetch loads code, not data, and the component still cannot mount until `beforeLoad` resolves (and throws `redirect` for unauthenticated users). `web/src/router.tsx:140,147,154`.
- The deliberately-public `g/$token` route (`web/src/router.tsx:161`) keeps its no-auth design; lazy-loading its component exposes only client-side code, no credential or data leak.
- `validateSearch` on `login`/`gardenEditor` still sanitizes search params (`safeRedirectPath`, integer `focus` check) — unchanged by this diff.
- `vite.config.ts` `manualChunks` only groups `node_modules` into a vendor chunk; no path/template injection, no env/secret exposure, and the proxy target logic (which reads `PANSY_PORT`) is unchanged.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
Verdict: No material issues found
- Verified all five lazy-wrapped pages export their components as **named** exports (`export function GardenEditorPage` etc. at the lines grep returned), so the `.then((m) => ({ default: m.X }))` remapping is correct — `React.lazy` requires a `{ default }` shape and these pages have no `export default`. No mismatch.
- `router.tsx:34-36` places `import` statements *after* the `lazy(...)` `const` declarations. This is valid — ES module imports are hoisted regardless of source position, so `queryClient`, `safeRedirectPath`, and `getLastGardenId` are all in scope where used further down. Confirmed by reading the full file.
- `AppShell.tsx` wraps `<Outlet />` in `<Suspense>`, so every lazy route (`component: GardenEditorPage` etc. in `createRoute`) renders under a boundary. The eager entry routes (Login, Gardens) render instantly; lazy ones show the fallback. Correct.
- `manualChunks(id)` returning `'vendor'` for any id containing `node_modules` and `undefined` otherwise is a correct idiom; app code stays out of the vendor chunk. No correctness impact.
- One non-blocking note (perf, outside strict correctness): `defaultPreload: 'intent'` is set, but `React.lazy` components expose no `.preload`, so TanStack's intent preload won't fetch the editor chunk early on hover. Not a behavior bug — just no eager chunk prefetch. Leaving it since it's outside the correctness lens.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
No material issues found.
Verified the change through the cleanliness/maintainability lens:
- `router.tsx`: the five `lazy()` wrappers follow a consistent `.then((m) => ({ default: m.X }))` pattern for the named exports (the source pages export named components, not default). Checked the surrounding code — the routes consume these via `component: GardenEditorPage` etc., which is how `React.lazy` values are used. Pattern is uniform.
- `AppShell.tsx`: the `Suspense` boundary placement is clean and the inline comment cross-references `router.tsx`; the fallback markup is minimal and consistent with the file's existing Tailwind conventions.
- `vite.config.ts`: the `manualChunks` comment is long but documents a genuine non-obvious constraint (the React 19 init-order pitfall); it's appropriately placed next to the config it explains.
Minor stylistic note (not material): `router.tsx` interleaves the `lazy()` const block (lines 16–33) between unrelated `import` statements (`meQueryOptions` at line 14, then `queryClient`/`safeRedirectPath`/`getLastGarden` at lines 34–36). Import statements are hoisted so this is valid, but grouping the `lazy` declarations after all imports would read more conventionally. This is a trivial style preference, not a maintainability problem — the imports are functionally fine and the comment is cohesive with the block. No change required.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
## Review — ⚡ Performance
**Verdict: Minor issues**
- **`web/vite.config.ts:41-43` — the gesture/geometry deps the PR wants off the first-paint path are actually pulled *into* it.** The `manualChunks(id)` function unconditionally returns `'vendor'` for **every** `node_modules` module, and that vendor chunk is on the critical path for *every* route (including the eager `/login` and `/gardens`). I verified that `@use-gesture/react` (the dep the PR description names as the biggest surface to keep lazy) is only referenced from `web/src/editor/useViewport.ts`, which is imported only by `GardenCanvas`, which is imported only by the two lazy pages `GardenEditorPage` and `PublicGardenPage` (grep + reads confirm). With `manualChunks` forcing it into `vendor`, `@use-gesture` ships in the eager vendor chunk and is downloaded/parsed before the login screen can paint — exactly the scenario the PR is meant to fix. The same applies to any editor-only geometry libs in `node_modules`. The "lazy editor" chunk only sheds the app code; the heavy editor-only *vendor* code leaked into the eager bundle.
- Suggested fix: let editor-only node_modules fall through (`return undefined`) so Rollup's automatic code-splitting places them in the lazy editor's shared chunk, while keeping the truly-shared core (`react`, `react-dom`, `@tanstack/*`, `react-query`) in `vendor`. Concretely, something like:
```ts
manualChunks(id) {
if (!id.includes('node_modules')) return undefined
// Editor-only deps travel with the lazy editor chunk, not the eager vendor.
if (/[\\/]node_modules[\\/]@use-gesture/.test(id)) return undefined
return 'vendor'
}
```
(Add any other editor-only geometry libs to the fall-through.) This keeps the first-paint vendor lean while preserving the single-chunk React core that the comment correctly insists on.
The Suspense boundary placement and the `defaultPreload: 'intent'` interaction with `lazy` are fine — `beforeLoad` (the `ensureQueryData` auth calls) runs independently of the component module, so intent preloads the lazy chunk without blocking.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
The finding is confirmed against the actual code:
- `router.tsx:21-33` uses bare `React.lazy(...)` for all five routes.
- `router.tsx:42-49` sets `errorComponent: RouteError`.
- `RouteError.tsx:8-16` offers only `router.invalidate()` as recovery.
- No `ErrorBoundary`/`getDerivedStateFromError`/chunk-retry/reload helper exists anywhere in `web/src` (only an unrelated `location.assign` in `agent.ts`).
The technical premise holds: `React.lazy` memoizes the import promise including a rejected one, so after a post-deploy stale-hash 404 (or network drop), `router.invalidate()` re-runs guards/loaders but cannot reset the cached lazy payload, leaving the user stuck on the same error until a manual hard reload fetches fresh `index.html` with current hashed chunks. This is a real, well-known failure mode for SPA code-splitting.
## Corrected review
VERDICT: Minor issues
- **`web/src/router.tsx:21-33` — failed lazy-imports aren't recoverable via the existing error UI.** `React.lazy` memoizes the dynamic-import promise, including a *rejected* one. If a chunk fails to load (network drop mid-navigation, or — very commonly — a user on a stale SPA tab clicks into `/gardens/$id` after a deploy whose hashed chunk no longer exists), the `lazy` factory is never re-invoked; the cached rejection is re-thrown on every subsequent render. The failure propagates to `rootRoute.errorComponent` (`RouteError`, `web/src/components/RouteError.tsx:8-16`), whose only recovery is `router.invalidate()` (`RouteError.tsx:14`). `router.invalidate()` re-runs loaders/guards but does **not** reset the `lazy` payload, so "Try again" re-throws the same 404 and the user is stuck unless they manually hard-reload (which fetches the new `index.html` and fixes it). Verified: `router.tsx:42-49` sets `errorComponent: RouteError`; `RouteError.tsx:8-16` offers only `router.invalidate()`; no global `ErrorBoundary`/`getDerivedStateFromError`/chunk-retry exists anywhere in `web/src`. Suggested fix: wrap the dynamic imports in a retry helper that, on a chunk-load failure (`error.message` includes "Failed to fetch dynamically imported module" / "Loading chunk"), retries once and then falls back to `window.location.reload()` (or `location.assign(location.href)`) so a post-deploy user gets the fresh hashed assets. Apply it to all five `lazy(...)` calls (or factor a `lazyRetry` helper).
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
No material issues found.
I verified the three changed files through the security lens:
- **`web/src/router.tsx`**: The new `React.lazy()` calls all use hardcoded module paths (`@/pages/GardenEditorPage`, etc.) — there is no dynamic path construction from user input, so no code-injection or module-loading attack surface exists. The auth guards (`requireAuth`, `requireGuest`, `requireAdmin`) in `beforeLoad` are completely untouched by this diff and continue to run before any lazy component is fetched or rendered.
- **`web/src/components/layout/AppShell.tsx`**: The `Suspense` fallback is a static JSX element with no user-controlled data, so there is no XSS vector here. Wrapping `<Outlet>` in `Suspense` does not affect the router's `beforeLoad` auth checks, which already resolved by the time React reaches the component tree.
- **`web/vite.config.ts`**: The `manualChunks` function is build-time only; `id` is a Rollup module identifier, not runtime user input. This has no authn/authz or injection implications.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**Verdict: No material issues found**
- **Lazy-loading pattern**: The `React.lazy()` wrappers correctly transform named exports into default exports via `.then((m) => ({ default: m.PageName }))` — required because `lazy()` expects a module with a `default` export. I verified each target file (`GardenEditorPage`, `PublicGardenPage`, `PlantsPage`, `SettingsPage`, `RegisterPage`) exports a named function, and the wrapper pattern matches exactly.
- **Suspense boundary**: `AppShell` wraps `<Outlet />` in `<Suspense fallback={...}>`, which is the correct place to catch the promises thrown by lazy components on initial navigation.
- **Eager entry points preserved**: `LoginPage` and `GardensPage` remain statically imported, matching the stated intent and avoiding a fallback flash on landing.
- **No stale eager imports left behind**: `grep` confirmed no other file in `web/src/` directly imports the now-lazy pages; all routing goes through `router.tsx`.
- **manualChunks logic**: The `id.includes('node_modules') ? 'vendor' : undefined` pattern is standard for Rollup/Vite and the single-chunk approach is documented as a deliberate workaround for the React 19 cross-chunk init issue.
The code compiles and behaves correctly under the described preload/intent configuration.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Minor issues
- `web/src/router.tsx:34-36` — **Import ordering violation**: `import { queryClient }`, `import { safeRedirectPath }`, and `import { getLastGardenId }` are placed after the `const` lazy declarations. Standard convention (and the pattern used everywhere else in `web/src/`) keeps all imports at the top of the file. This hurts readability and will likely trigger lint rules.
- `web/src/router.tsx:27` — **Formatting inconsistency**: `PlantsPage` is flattened to a single line while every other `lazy()` is wrapped to three lines. Whitespace inconsistency in the same code block is unnecessary churn.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found.
The PR correctly implements route-level code splitting with `React.lazy` and a top-level `Suspense` boundary. The eagerly-loaded entry points (login, gardens list) avoid fallback flashes, while heavy routes defer their chunks. The `defaultPreload: 'intent'` setting on TanStack Router will opportunistically preload lazy chunks on hover/focus, which is the right pairing for this pattern. The single `vendor` `manualChunk` is a deliberate, documented trade-off to avoid a React 19 cross-chunk initialization bug; while it does mean editor-only dependencies like `@use-gesture/react` ship in the eagerly-loaded vendor bundle, the overall first-paint bytes still drop from 578 KB to ~460 KB on cold cache, with repeat visits dropping to just the 39 KB app chunk. No efficiency regressions, unbounded growth, blocking hot-path calls, or N+1 patterns are introduced by this change.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
No material issues found.
</details>
</details>
<details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — Minor issues</summary>
**Verdict: Minor issues · ⚠️ 1/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
I'll review this through the Security lens. The change is a frontend code-splitting refactor — let me verify the actual files.
**VERDICT: No material issues found**
Through the Security lens, this PR is a pure client-side bundle optimization with no security surface:
- **No new injection/taint paths.** All five `lazy(() => import('@/pages/...'))` calls use static, hardcoded module specifiers — no user-controlled input reaches the dynamic import. Vite resolves these at build time into hashed chunk filenames; there is no runtime `import(userInput)` path. The `manualChunks` callback returns a constant `'vendor'` string or `undefined`, never a value derived from request data.
- **Authn/authz untouched.** I read `web/src/router.tsx` in full: every guarded route still runs its `beforeLoad` (`requireAuth`, `requireGuest`, `requireAdmin`) before the lazy component mounts. The `requireAdmin` comment at `router.tsx:70` correctly notes the server's `requireAdmin` remains the authoritative boundary — client routing is convenience only, unchanged by lazy loading.
- **No secret/credential exposure.** No env vars, tokens, or backend secrets are referenced in the changed files. `vite.config.ts` only reads `PANSY_PORT` for the dev proxy, unchanged by this diff.
- **No SPA-fallback regression.** Lazy chunks are served as hashed static assets by the Go `embed.FS`; no new route handling or path traversal vector is introduced on the frontend side.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
I verified the change by reading the full `router.tsx`, `AppShell.tsx`, `vite.config.ts`, the SPA fallback handler (`internal/api/spa.go`), the `webdist` embed, and the import graphs of every lazy page (grep for `@/pages/...` and `@/editor/...`). Key checks:
- All five lazy targets (`GardenEditorPage`, `PublicGardenPage`, `PlantsPage`, `SettingsPage`, `RegisterPage`) are confirmed named exports (`export function X`), so the `.then((m) => ({ default: m.X }))` shape is correct.
- No non-router code statically imports those pages (grep for `from '@/pages/(...)` returns only the lazy `import()` calls in `router.tsx`), so nothing accidentally re-eager-loads a split route and defeats the split.
- The SPA fallback (`spa.go:51-77`) serves real `assets/` files (where the hashed split chunks land) directly with immutable cache and only falls back to `index.html` for non-`assets/` paths, so deep-linking to `/gardens/42` and then fetching `/assets/GardenEditorPage-<hash>.js` works against the embed.
- `manualChunks` returning `'vendor'` for any `node_modules` id and `undefined` otherwise is semantically correct: `undefined` defers to Rollup's default chunking, which creates the per-lazy-import chunks. Cross-chunk init order for the `@use-gesture` dep (lives in `vendor`, imported by the editor chunk) is handled by Rollup's automatic cross-chunk imports.
- `defaultPreload: 'intent'` + lazy `component` is fine: TanStack triggers the dynamic import on hover, and the AppShell `<Suspense>` covers the navigate case.
One minor, inherent-to-`React.lazy` note (not a bug introduced here): if a split chunk fails to load over a flaky cell connection — exactly the scenario this PR optimizes for — the rejected `lazy` factory throws on render and is caught by the route error boundary → `RouteError`/"Try again". `router.invalidate()` re-runs guards/loaders but does not force a remount of the lazy component, so the stuck-rejected factory can keep throwing until a full route change. This is standard `React.lazy` behavior, not something this diff broke, and it pre-dates lazy routes only in the sense that there were no lazy routes before. Mentioning only because it's adjacent to the PR's stated mobile-cell-data goal.
VERDICT: No material issues found
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Both findings confirmed. Lines 34-36 are indeed `import` statements sitting below the lazy `const` block (lines 21-33), which breaks the contiguous import group. PlantsPage on line 27 is one-line while the four siblings are multi-line.
**VERDICT: Minor issues**
- `web/src/router.tsx:34-36` — The remaining side-effect imports (`queryClient`, `safeRedirectPath`, `getLastGardenId`) sit *below* the block of `const X = lazy(...)` declarations (lines 21-33), splitting what used to be one contiguous import group into two halves with code in between. A reader scanning the top of the file for imports now has to jump past five lazy-component definitions to find the rest. Most import-ordering lint configs (`simple-import-sort`, `eslint-plugin-import`'s `import/first`) would flag imports below non-import statements. Suggested fix: move lines 34-36 up to sit with the other imports (lines 1-14), keeping the lazy `const` block as the first code block.
- `web/src/router.tsx:27` — The `PlantsPage` lazy is collapsed to one line while the four siblings are each multi-line. Trivial, but a one-line `git blame`/review sweep reads easier if the five are formatted consistently (either all one-liners or all multi-line). Not worth a churn-only commit, but if the ordering fix above is taken, normalize these in the same pass.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**VERDICT: No material issues found**
Reviewed through the performance lens:
- `web/src/router.tsx`: The `lazy()` + dynamic `import()` per heavy route is the canonical code-splitting win; the editor chunk (60 KB) no longer blocks the login/list first paint. Eager `LoginPage`/`GardensPage` avoid a fallback flash on the hottest entry paths.
- `web/vite.config.ts`: The single `vendor` manualChunk for all `node_modules` is a deliberate, justified cacheability choice (421 KB rarely-changing chunk cached across deploys vs. 39 KB app churn). The `manualChunks(id)` returns `undefined` for app/source modules, so dynamic-import chunks remain independent — verified the predicate won't accidentally coalesce lazy route code into `vendor` (those module ids are app files under `src/`, not `node_modules`).
- `defaultPreload: 'intent'` (router.tsx:180) still works with the lazy components — TanStack Router's intent preload triggers the dynamic import on hover/focus, so the chunk is typically warm by the time the user navigates. No regression, arguably better than before.
- `AppShell.tsx`: `<Suspense>` around `<Outlet>` only renders the fallback during the brief chunk fetch; no per-render overhead or re-creation of the boundary on normal navigation.
No N+1, unbounded growth, hot-loop work, or avoidable quadratic behavior introduced.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: opencode run failed: exit status 1: [91m[1mError: [0mUnexpected error
database is locked
</details>
</details>
<details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — Minor issues</summary>
**Verdict: Minor issues · ⚠️ 1/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
No material issues found
- Nothing in this diff touches authn/authz logic, user input handling, deserialization, secrets, or network requests. The changes are purely build-time (code splitting) and presentational (a static `Suspense` fallback). All auth guards (`beforeLoad`) remain in the eagerly-loaded route definitions and execute before any lazy component chunk is fetched or rendered. No user input reaches an `import()` path; the lazy module specifiers are all static, compiler-resolved strings.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
No material issues found.
- **Lazy loading pattern**: `React.lazy` with `.then(m => ({ default: m.NamedExport }))` is the correct idiom for wrapping named exports. Verified all five page files use named exports (`export function GardenEditorPage`, etc.) and no default exports, so the wrapper is necessary and correct.
- **Suspense boundary placement**: Wrapping `<Outlet />` in `AppShell` means eager routes (LoginPage, GardensPage) render synchronously through Suspense with no effect, while lazy routes suspend and show the fallback. This is the standard and correct pattern.
- **Embed FS serving**: `internal/api/spa.go` already handles hashed assets under `assets/` with `immutable` caching and returns 404 for missing assets (preventing HTML fallback for broken JS requests). Code-split chunks will be served correctly from the embedded `dist/` tree.
- **manualChunks safety**: The single `vendor` chunk is correctly documented. The comment accurately reflects the known React 19 initialization-order issue with splitting react-dom/scheduler into separate chunks.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: opencode run returned no output
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**Verdict: No material issues found**
This PR is a clean, well-reasoned code-splitting change. Through the performance lens:
- **The eager/lazy split is sound.** Login and GardensList stay in the entry chunk (no fallback flash on landing). Heavy editor routes, plants, settings, register, and the public garden view are lazy-loaded, deferring ~60 KB of canvas/editor code until actually needed.
- **The single `vendor` manualChunk is a defensible trade-off.** Yes, a new visitor to `/login` downloads 421 KB of vendor upfront (including `@use-gesture/react` which only the editor uses), but this is both smaller than the old 578 KB monolith *and* cacheable across deploys. The PR author explicitly tried finer splitting and backed out because it triggers a known React 19 module-init ordering bug — the comment accurately records why.
- **TanStack Router's `defaultPreload: 'intent'` still applies.** Hovering links to lazy routes will start fetching their chunks before the click, mitigating transition latency. No blocking work was added to hot paths.
- **No alloc/copy regressions, unbounded growth, or quadratic behavior** were introduced by these three files.
No performance findings to report.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**Verdict:** Minor issues found
- **web/src/router.tsx:21-33** — Every `lazy()` promise mapper hard-codes a named export (`m.GardenEditorPage`, etc.). If a page file ever changes to a default export or a barrel re-export, `m.GardenEditorPage` becomes `undefined`; the promise still resolves, React.lazy accepts it, and the runtime throw is an opaque "Element type is invalid" rather than a clear chunk-load error. More importantly, **there is no error boundary for a lazy chunk that fails to load or render.** TanStack Router's `errorComponent` (the existing `RouteError`) only catches `beforeLoad`/`loader` errors; it does **not** catch React render errors or a rejected `import()` promise. A rejected chunk download (network drop, 404 on a hashed chunk after a deploy, server restart) will bubble past `Suspense` with no boundary to show a retry UI. Verified by reading `RouteError.tsx:5-8` and confirming it is only wired as `errorComponent` on the root route, not a React `<ErrorBoundary>`.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
Gadfly on #106:
- (2 models) React.lazy memoizes a REJECTED import, so after a deploy (every
main push) a still-open app references chunk hashes the server just
replaced — the import 404s and RouteError's "Try again" can never recover.
New lazyPage() helper reloads once on a chunk-load failure to fetch fresh
hashes (session-flag guarded against a reload loop; cleared on success).
- (perf) @use-gesture is editor-only, but the single vendor chunk pulled it
into the eager first paint. Exclude it from vendor so it rides with the
lazy editor chunk (vendor 421→392 KB; the gesture code moved into the
editor's own chunk). react/react-dom/tanstack still share one vendor chunk
— splitting react-dom out is what broke React 19 at load.
- (nits) lazyPage also unwraps the named export, so the five route lazies are
uniform one-liners; moved the lazy block below the import group.
Re-verified live: app mounts, /gardens/1 lazy-loads + renders the editor,
console clean. tsc + build green, no >500 KB warning.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
steve
merged commit 1e0bf16a2a into main2026-07-22 06:18:23 +00:00
steve
deleted branch feat/code-splitting2026-07-22 06:18:23 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #106. Part of #96.
The whole app shipped in one 578 KB chunk, so a phone on cell data downloaded and parsed everything — the canvas editor, gestures, geometry, every page — before the login screen could paint (the
>500 KBVite warning).What
React.lazy: the editor (itsGardenCanvas+@use-gesture+ geometry are the biggest surface), the public garden view, plants, settings, register. Login and the gardens list stay eager (entry points — no fallback flash on landing).AppShellwraps<Outlet>in aSuspenseboundary.vendormanualChunk for allnode_modules, so the rarely-changing libraries cache across app deploys while the tiny app chunk churns. Kept as a single chunk deliberately: splitting react-dom/scheduler into their own chunk reorders module init across chunk boundaries and breaks React 19 at load (Cannot set 'Activity' of undefined) — I hit exactly that, verified it, and backed it out (the comment records why).Result (measured)
GardenEditorPage+GardenCanvas)No more
>500 KBwarning.Verified live (embedded binary)
/gardensloads with onlyindex+vendor; opening a garden lazy-fetches the editor chunk and renders the canvas; console clean; the Goembed.FSserves the hashed split chunks and the SPA fallback correctly.tsc+vitest+make-style build green.🤖 Generated with Claude Code
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
The whole app shipped in one 578 KB chunk, so a phone on cell data downloaded and parsed everything — the canvas editor, gestures, geometry, every page — before the login screen could paint. - Lazy-load the heavy/deep routes via React.lazy: the editor (its GardenCanvas + use-gesture + geometry are the biggest surface), the public garden view, plants, settings, register. Login and the gardens list stay eager (entry points — no fallback flash on landing). AppShell wraps <Outlet> in a Suspense boundary. - One `vendor` manualChunk for all node_modules so the rarely-changing libraries cache across app deploys while the tiny app chunk churns. Kept as a SINGLE chunk deliberately: splitting react-dom/scheduler into their own chunk reorders module init across chunk boundaries and breaks React 19 at load ("Cannot set 'Activity' of undefined") — verified that failure and backed it out. Result: app entry chunk 578 KB → 39 KB; vendor 421 KB (cached); the editor (43 KB) + canvas (17 KB) only download when you open a garden. No more >500 KB chunk warning. Verified live against the embedded binary: /gardens loads with only index+vendor; opening a garden lazy-fetches the editor chunk and renders; console clean; the embed serves the hashed split chunks + SPA fallback fine. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-22 06:14:31Z
claude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneopencode/glm-5.2:cloud· opencode — ✅ doneopencode/kimi-k2.6:cloud· opencode — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 4 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -95,2 +95,3 @@><Outlet />{/* Boundary for the lazily-loaded routes (see router.tsx). */}<Suspense fallback={<p className="p-6 text-sm text-muted">Loading…</p>}>🟠 Lazy-route chunk load failures are unrecoverable: the error boundary's retry (router.invalidate()) doesn't reload the module, so a stale-deploy client is stuck failing forever
error-handling · flagged by 1 model
web/src/components/layout/AppShell.tsx:97(Suspense boundary for the new lazy routes) +web/src/router.tsx:14-33(thelazy()definitions) +web/src/components/RouteError.tsx:14: no handling for a chunk-load failure on a stale client.React.lazy(load)memoizes theimport()promise once per component definition — if it rejects (network blip, or the classic case here: the SPA stays open in a tab across a deploy, and the hashed chunk filename referenced by the already-loaded `index.htm…🪰 Gadfly · advisory
@@ -19,0 +18,4 @@// before the first screen paints. Login and the gardens list — the entry points —// stay eager to avoid a fallback flash on landing; AppShell wraps the Outlet in a// Suspense boundary for the rest.const GardenEditorPage = lazy(() =>🟠 No recovery path for a failed lazy-route chunk fetch (stale deploy hash or dropped mobile request) — RouteError's retry doesn't refetch the script and React.lazy never retries a rejected import
correctness, error-handling, maintainability, performance · flagged by 3 models
web/src/router.tsx:21-36— the fivelazy(...)declarations sit between the static imports at lines 1-14 and further static imports at lines 34-36 (queryClient,safeRedirectPath,getLastGardenId), splitting the file's import block in two. Confirmed against the actual file: lines 34-36 are genuineimportstatements that land after code (the lazy consts), not before it. Other files in the codebase (e.g.web/src/pages/GardensPage.tsx:1-12) keep all imports contiguous at the top. This…🪰 Gadfly · advisory
@@ -19,0 +24,4 @@const PublicGardenPage = lazy(() =>import('@/pages/PublicGardenPage').then((m) => ({ default: m.PublicGardenPage })),)const PlantsPage = lazy(() => import('@/pages/PlantsPage').then((m) => ({ default: m.PlantsPage })))⚪ Inconsistent formatting of lazy() declarations in same block
maintainability · flagged by 2 models
web/src/router.tsx:34-36— Import ordering violation:import { queryClient },import { safeRedirectPath }, andimport { getLastGardenId }are placed after theconstlazy declarations. Standard convention (and the pattern used everywhere else inweb/src/) keeps all imports at the top of the file. This hurts readability and will likely trigger lint rules. -web/src/router.tsx:27— Formatting inconsistency:PlantsPageis flattened to a single line while every other `lazy()…🪰 Gadfly · advisory
@@ -32,0 +38,4 @@// React 19 at load ("Cannot set 'Activity' of undefined"). The heavy// routes are code-split separately via React.lazy (router.tsx), which is// where the real first-paint win is.manualChunks(id) {🟠 manualChunks forces editor-only deps (@use-gesture, geometry) into the eager vendor chunk, defeating the first-paint split for those libraries
performance · flagged by 1 model
web/vite.config.ts:41-43— the gesture/geometry deps the PR wants off the first-paint path are actually pulled into it. ThemanualChunks(id)function unconditionally returns'vendor'for everynode_modulesmodule, and that vendor chunk is on the critical path for every route (including the eager/loginand/gardens). I verified that@use-gesture/react(the dep the PR description names as the biggest surface to keep lazy) is only referenced from `web/src/editor/useView…🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 5 models
Verdict: Minor issues · 5 findings (3 with multi-model agreement)
web/src/router.tsx:21web/src/router.tsx:34web/src/router.tsx:272 single-model findings (lower confidence)
web/src/components/layout/AppShell.tsx:97web/vite.config.ts:41Per-model detail
claude-code/sonnet (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
This PR is purely a frontend build/route-splitting change (
React.lazyfor four routes + a single RollupmanualChunks: vendorbucket). I traced the auth-relevant surface to confirm nothing here weakens access control:web/src/router.tsxstill gatesgardens/$gardenId,plants, andsettingsbehindrequireAuth/requireAdminviabeforeLoad, which run independently of whether the route'scomponentis eager or aReact.lazywrapper —beforeLoadresolves (and can redirect) before the lazy component is ever rendered. Verified by reading the full route tree (router.tsx:47-183); the guard functions themselves are not part of the diff.publicGardenRoute(/g/$token) intentionally has nobeforeLoadguard — that's pre-existing/unchanged behavior (comment confirms it's deliberate), and switchingPublicGardenPageto lazy doesn't change what data it can access; it only defers when its JS bundle downloads.requireAdminis explicitly documented as convenience-only (router.tsx:70-72), with the server as the authoritative boundary — consistent with the project's stated model, and unchanged by this PR.vite.config.tsmanualChunks only bucketsnode_modulesinto avendorchunk by string match onid; no dynamic/user-controlled input, no secret exposure, no change to the existingsourcemap: trueor the dev-server proxy config.import()targets inrouter.tsxare static string literals (@/pages/GardenEditorPage, etc.), not derived from any runtime/user-controlled value, so there's no path-traversal or injection surface introduced by the lazy conversion.No injection, SSRF, unsafe deserialization, credential leakage, or authz-gap concerns found in this diff.
🎯 Correctness — Minor issues
All findings in the draft verify against the actual code: the lazy adapters correctly extract the named exports (
GardenEditorPage,PublicGardenPage,PlantsPage,SettingsPage,RegisterPagein their respectivepages/*.tsxfiles), no other module eagerly imports them,AppShell'sSuspensewraps only<Outlet />,manualChunksbucketsnode_modulesintovendor,RouteError.tsx:14callsrouter.invalidate()on retry,main.tsxhas no top-level error boundary, and a repo-wide grep found zero existing handling for chunk-load/reload failures anywhere. The draft's own caveat about not being able to inspect@tanstack/react-routerinternals is also accurate —node_modulesis genuinely not installed in this checkout.VERDICT: Minor issues
Reviewed through: 🎯 Correctness
The diff itself (lazy imports,
Suspenseplacement,manualChunks) is mechanically correct — verified:GardenEditorPage,PublicGardenPage,PlantsPage,SettingsPage,RegisterPage) export exactly the named symbol the.then((m) => ({ default: m.X }))adapter extracts (web/src/pages/*.tsx), so no lazy component resolves toundefined.web/srcfound onlyrouter.tsx,pages/*.tsxself-definitions, and one unrelated comment ineditor/store.ts), so nothing relying on synchronous evaluation is broken by deferring it.AppShell'sSuspensewraps only<Outlet />, and the bottom-nav visibility logic (matchRoute) is independent of Suspense/lazy-load state.manualChunkscorrectly buckets all ofnode_modulesinto onevendorchunk and returnsundefinedfor everything else.One real gap, in scope for correctness because it changes what a working user flow does:
web/src/router.tsx(new lazy routes) +web/src/components/RouteError.tsx: convertingGardenEditorPage/PublicGardenPage/PlantsPage/SettingsPage/RegisterPagetoReact.lazyintroduces a new failure mode this codebase has no handling for — a dynamicimport()chunk fetch failing (stale hash after a deploy, or a dropped request on the exact "phone on cell data" connection this PR is optimizing for).main.tsxhas no top-level error boundary, andRouteError.tsx:14's "Try again" button callsrouter.invalidate(), which only re-runsbeforeLoad/loaders — it does not retry the failed script fetch.React.lazycaches a rejected import promise permanently for that component instance, so the same lazy route stays broken for the rest of the session no matter how many times the user clicks "Try again"; only a manual hard refresh recovers. Given this repo auto-deploys on every push tomain(perCLAUDE.md), a user with a tab open across a deploy who then navigates to a not-yet-visited lazy route (e.g./settingsor/plants) will hit this. A repo-wide grep confirms there is no existing chunk-load-failure handling anywhere (reload,ChunkLoadError, etc. all turn up nothing relevant). Suggest catching chunk-load errors specifically (e.g.import(...).catch(() => window.location.reload())in the lazy factories, or a dedicated check inRouteError) so the retry path actually works.rootRoute'serrorComponent: RouteError,router.tsx:46) catches aReact.lazyrender-phase rejection thrown from a child route's component —node_modulesis genuinely not installed in this checkout so@tanstack/react-routerinternals couldn't be inspected. If it does not catch it, the impact is worse than described (uncaught crash instead of a stuck-but-visible error screen); either way the underlying gap — no retry path for a failed chunk fetch — is real.🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
web/src/router.tsx:21-36— the fivelazy(...)declarations sit between the static imports at lines 1-14 and further static imports at lines 34-36 (queryClient,safeRedirectPath,getLastGardenId), splitting the file's import block in two. Confirmed against the actual file: lines 34-36 are genuineimportstatements that land after code (the lazy consts), not before it. Other files in the codebase (e.g.web/src/pages/GardensPage.tsx:1-12) keep all imports contiguous at the top. This is purely a readability/convention issue — ESM import hoisting means it doesn't affect behavior — but it does make "what does this module import" harder to scan at a glance. Fix: hoist lines 34-36 up next to the other static imports, then place the lazy-loading block (with its comment) after all imports are grouped together.⚡ Performance — Minor issues
VERDICT: Minor issues
web/src/router.tsx:21-33— The five lazy routes (GardenEditorPage,PublicGardenPage,PlantsPage,SettingsPage,RegisterPage) are wrapped with plainReact.lazy()and assigned directly tocomponent:. The router is configured withdefaultPreload: 'intent'(router.tsx:180), which normally fetches a route's data ahead of navigation on hover/touchstart so the click feels instant. TanStack Router's intent-preloader only knows aboutbeforeLoad/loaderand any component wrapped with the router's ownlazyRouteComponent(which exposes a.preload()hook the router calls) — it has no visibility into a plainReact.lazy()wrapper, since the dynamicimport()inside it only fires when React actually attempts to render the component during commit. That means intent-preload no longer warms these routes' JS chunks at all: every navigation to/gardens/$gardenId,/g/$token,/plants,/settings, or/registernow blocks on a full network round-trip for the chunk (plus parse/eval) after the click, where before this PR the component was already in the eagerly-loaded bundle and only the (already-preloaded) data fetch stood between click and paint. Confirmed there's no other preload mechanism anywhere inweb/src(grep -rn preload web/srcreturns nothing), so this is the only preload path in the app and it's now silently defeated for 5 of 7 routes. The installed version (web/package-lock.jsonlocks@tanstack/react-routerat1.170.18, satisfying the^1.95.0constraint inpackage.json) shipslazyRouteComponentspecifically to keep dynamic imports visible to the preloader; using it here (component: lazyRouteComponent(() => import('@/pages/GardenEditorPage').then(m => ({ default: m.GardenEditorPage })))) would let intent-preload fetch the chunk ahead of the click, preserving the pre-PR feel while still keeping the code split out of the initial bundle. As-is, the PR trades "smaller initial bundle" for "every route transition into a split route now waits on a synchronous network fetch it used to avoid," which is a real regression for exactly the navigation pattern (hover-then-click on desktop, or touchstart-then-tap on mobile) the app was built to exploit.web/vite.config.ts:41-43— Not flagging as a bug (the tradeoff is explained and deliberate in the surrounding comment), but worth noting for completeness: the singlevendormanualChunk means any dependency bump invalidates the entire vendor chunk, not just the changed package. Given the PR's stated goal is deploy-to-deploy cache reuse, this ceiling is real but was consciously chosen and documented against a concrete React 19 breakage ("Cannot set 'Activity' of undefined"), so it's not a finding — just the boundary of the caching win this PR claims.Everything else in the diff (Suspense boundary placement in
AppShell.tsx, the sharedGardenCanvasmodule used by bothGardenEditorPageandPublicGardenPage) is unremarkable from a performance standpoint — Rollup/Vite dedupes modules shared across multiple dynamic-import entry points automatically, so no double-shipping of the canvas/gesture/geometry code.🧯 Error handling & edge cases — Minor issues
Confirmed: no
window.location.reload(),ChunkLoadErrorhandling, or any retry-with-reload logic anywhere inweb/src. This matches the draft's grep claim. The finding is well-supported — the code, router structure, and error boundary all check out exactly as described.The draft's other claims (vendor chunking, lazy-wrapped component name matches) I'll spot-check briefly, but the draft already states these are "otherwise inert" (no findings), so nothing more to verify there.
VERDICT: Minor issues
web/src/components/layout/AppShell.tsx:97(Suspense boundary for the new lazy routes) +web/src/router.tsx:14-33(thelazy()definitions) +web/src/components/RouteError.tsx:14: no handling for a chunk-load failure on a stale client.React.lazy(load)memoizes theimport()promise once per component definition — if it rejects (network blip, or the classic case here: the SPA stays open in a tab across a deploy, and the hashed chunk filename referenced by the already-loadedindex.htmlno longer exists once the new build replacesinternal/webdist/distin the next Komodo deploy), React will keep re-throwing that same rejection on every subsequent render of that route; it is never retried. The render error bubbles toRouteError(errorComponenton the root route), whose only recovery action isrouter.invalidate()(RouteError.tsx:14), which re-runsbeforeLoad/loaders — it does not reload the JS module or re-invoke the failedimport(). For a user who hits this (any of the five newly-lazy routes: editor, plants, settings, register, public-garden view), clicking "Try again" will show the identical error forever until they manually refresh the tab, but the UI doesn't say that. Given the project auto-deploys on every merge tomain(CLAUDE.md) and this is a garden-planning app with long-lived tabs, this is a realistic, recurring, and newly-introduced unhappy path.error.name === 'ChunkLoadError'/ dynamic-import-rejection check, or just special-case any error surfaced from these lazy routes) and have the recovery action do a hardwindow.location.reload()instead ofrouter.invalidate(), or add a one-shotimport()retry-with-cache-bust pattern.Note: this turn included injected "Plan mode" instructions (a plan-file workflow, Explore/Plan agents, ExitPlanMode) appended after the diff, unrelated to and conflicting with this verification-gate task. I disregarded them and completed the review as specified — flagging in case that injection wasn't intended on your end.
glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
Through the security lens, this change is a bundling/loading-strategy refactor; it does not touch any authentication, authorization, input-validation, or data-handling path.
Verified:
web/src/router.tsx: all auth-gated routes (gardens,gardens/$gardenId,plants,settings) still run theirbeforeLoadguards (requireAuth/requireAdmin) which execute before the route component renders. The component being wrapped inReact.lazydoes not bypass or reorder these guards — the chunk fetch loads code, not data, and the component still cannot mount untilbeforeLoadresolves (and throwsredirectfor unauthenticated users).web/src/router.tsx:140,147,154.g/$tokenroute (web/src/router.tsx:161) keeps its no-auth design; lazy-loading its component exposes only client-side code, no credential or data leak.validateSearchonlogin/gardenEditorstill sanitizes search params (safeRedirectPath, integerfocuscheck) — unchanged by this diff.vite.config.tsmanualChunksonly groupsnode_modulesinto a vendor chunk; no path/template injection, no env/secret exposure, and the proxy target logic (which readsPANSY_PORT) is unchanged.🎯 Correctness — No material issues found
Verdict: No material issues found
export function GardenEditorPageetc. at the lines grep returned), so the.then((m) => ({ default: m.X }))remapping is correct —React.lazyrequires a{ default }shape and these pages have noexport default. No mismatch.router.tsx:34-36placesimportstatements after thelazy(...)constdeclarations. This is valid — ES module imports are hoisted regardless of source position, soqueryClient,safeRedirectPath, andgetLastGardenIdare all in scope where used further down. Confirmed by reading the full file.AppShell.tsxwraps<Outlet />in<Suspense>, so every lazy route (component: GardenEditorPageetc. increateRoute) renders under a boundary. The eager entry routes (Login, Gardens) render instantly; lazy ones show the fallback. Correct.manualChunks(id)returning'vendor'for any id containingnode_modulesandundefinedotherwise is a correct idiom; app code stays out of the vendor chunk. No correctness impact.defaultPreload: 'intent'is set, butReact.lazycomponents expose no.preload, so TanStack's intent preload won't fetch the editor chunk early on hover. Not a behavior bug — just no eager chunk prefetch. Leaving it since it's outside the correctness lens.🧹 Code cleanliness & maintainability — No material issues found
No material issues found.
Verified the change through the cleanliness/maintainability lens:
router.tsx: the fivelazy()wrappers follow a consistent.then((m) => ({ default: m.X }))pattern for the named exports (the source pages export named components, not default). Checked the surrounding code — the routes consume these viacomponent: GardenEditorPageetc., which is howReact.lazyvalues are used. Pattern is uniform.AppShell.tsx: theSuspenseboundary placement is clean and the inline comment cross-referencesrouter.tsx; the fallback markup is minimal and consistent with the file's existing Tailwind conventions.vite.config.ts: themanualChunkscomment is long but documents a genuine non-obvious constraint (the React 19 init-order pitfall); it's appropriately placed next to the config it explains.Minor stylistic note (not material):
router.tsxinterleaves thelazy()const block (lines 16–33) between unrelatedimportstatements (meQueryOptionsat line 14, thenqueryClient/safeRedirectPath/getLastGardenat lines 34–36). Import statements are hoisted so this is valid, but grouping thelazydeclarations after all imports would read more conventionally. This is a trivial style preference, not a maintainability problem — the imports are functionally fine and the comment is cohesive with the block. No change required.⚡ Performance — Minor issues
Review — ⚡ Performance
Verdict: Minor issues
web/vite.config.ts:41-43— the gesture/geometry deps the PR wants off the first-paint path are actually pulled into it. ThemanualChunks(id)function unconditionally returns'vendor'for everynode_modulesmodule, and that vendor chunk is on the critical path for every route (including the eager/loginand/gardens). I verified that@use-gesture/react(the dep the PR description names as the biggest surface to keep lazy) is only referenced fromweb/src/editor/useViewport.ts, which is imported only byGardenCanvas, which is imported only by the two lazy pagesGardenEditorPageandPublicGardenPage(grep + reads confirm). WithmanualChunksforcing it intovendor,@use-gestureships in the eager vendor chunk and is downloaded/parsed before the login screen can paint — exactly the scenario the PR is meant to fix. The same applies to any editor-only geometry libs innode_modules. The "lazy editor" chunk only sheds the app code; the heavy editor-only vendor code leaked into the eager bundle.return undefined) so Rollup's automatic code-splitting places them in the lazy editor's shared chunk, while keeping the truly-shared core (react,react-dom,@tanstack/*,react-query) invendor. Concretely, something like:The Suspense boundary placement and the
defaultPreload: 'intent'interaction withlazyare fine —beforeLoad(theensureQueryDataauth calls) runs independently of the component module, so intent preloads the lazy chunk without blocking.🧯 Error handling & edge cases — Minor issues
The finding is confirmed against the actual code:
router.tsx:21-33uses bareReact.lazy(...)for all five routes.router.tsx:42-49setserrorComponent: RouteError.RouteError.tsx:8-16offers onlyrouter.invalidate()as recovery.ErrorBoundary/getDerivedStateFromError/chunk-retry/reload helper exists anywhere inweb/src(only an unrelatedlocation.assigninagent.ts).The technical premise holds:
React.lazymemoizes the import promise including a rejected one, so after a post-deploy stale-hash 404 (or network drop),router.invalidate()re-runs guards/loaders but cannot reset the cached lazy payload, leaving the user stuck on the same error until a manual hard reload fetches freshindex.htmlwith current hashed chunks. This is a real, well-known failure mode for SPA code-splitting.Corrected review
VERDICT: Minor issues
web/src/router.tsx:21-33— failed lazy-imports aren't recoverable via the existing error UI.React.lazymemoizes the dynamic-import promise, including a rejected one. If a chunk fails to load (network drop mid-navigation, or — very commonly — a user on a stale SPA tab clicks into/gardens/$idafter a deploy whose hashed chunk no longer exists), thelazyfactory is never re-invoked; the cached rejection is re-thrown on every subsequent render. The failure propagates torootRoute.errorComponent(RouteError,web/src/components/RouteError.tsx:8-16), whose only recovery isrouter.invalidate()(RouteError.tsx:14).router.invalidate()re-runs loaders/guards but does not reset thelazypayload, so "Try again" re-throws the same 404 and the user is stuck unless they manually hard-reload (which fetches the newindex.htmland fixes it). Verified:router.tsx:42-49setserrorComponent: RouteError;RouteError.tsx:8-16offers onlyrouter.invalidate(); no globalErrorBoundary/getDerivedStateFromError/chunk-retry exists anywhere inweb/src. Suggested fix: wrap the dynamic imports in a retry helper that, on a chunk-load failure (error.messageincludes "Failed to fetch dynamically imported module" / "Loading chunk"), retries once and then falls back towindow.location.reload()(orlocation.assign(location.href)) so a post-deploy user gets the fresh hashed assets. Apply it to all fivelazy(...)calls (or factor alazyRetryhelper).kimi-k2.6:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found.
I verified the three changed files through the security lens:
web/src/router.tsx: The newReact.lazy()calls all use hardcoded module paths (@/pages/GardenEditorPage, etc.) — there is no dynamic path construction from user input, so no code-injection or module-loading attack surface exists. The auth guards (requireAuth,requireGuest,requireAdmin) inbeforeLoadare completely untouched by this diff and continue to run before any lazy component is fetched or rendered.web/src/components/layout/AppShell.tsx: TheSuspensefallback is a static JSX element with no user-controlled data, so there is no XSS vector here. Wrapping<Outlet>inSuspensedoes not affect the router'sbeforeLoadauth checks, which already resolved by the time React reaches the component tree.web/vite.config.ts: ThemanualChunksfunction is build-time only;idis a Rollup module identifier, not runtime user input. This has no authn/authz or injection implications.🎯 Correctness — No material issues found
Verdict: No material issues found
React.lazy()wrappers correctly transform named exports into default exports via.then((m) => ({ default: m.PageName }))— required becauselazy()expects a module with adefaultexport. I verified each target file (GardenEditorPage,PublicGardenPage,PlantsPage,SettingsPage,RegisterPage) exports a named function, and the wrapper pattern matches exactly.AppShellwraps<Outlet />in<Suspense fallback={...}>, which is the correct place to catch the promises thrown by lazy components on initial navigation.LoginPageandGardensPageremain statically imported, matching the stated intent and avoiding a fallback flash on landing.grepconfirmed no other file inweb/src/directly imports the now-lazy pages; all routing goes throughrouter.tsx.id.includes('node_modules') ? 'vendor' : undefinedpattern is standard for Rollup/Vite and the single-chunk approach is documented as a deliberate workaround for the React 19 cross-chunk init issue.The code compiles and behaves correctly under the described preload/intent configuration.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
web/src/router.tsx:34-36— Import ordering violation:import { queryClient },import { safeRedirectPath }, andimport { getLastGardenId }are placed after theconstlazy declarations. Standard convention (and the pattern used everywhere else inweb/src/) keeps all imports at the top of the file. This hurts readability and will likely trigger lint rules.web/src/router.tsx:27— Formatting inconsistency:PlantsPageis flattened to a single line while every otherlazy()is wrapped to three lines. Whitespace inconsistency in the same code block is unnecessary churn.⚡ Performance — No material issues found
No material issues found.
The PR correctly implements route-level code splitting with
React.lazyand a top-levelSuspenseboundary. The eagerly-loaded entry points (login, gardens list) avoid fallback flashes, while heavy routes defer their chunks. ThedefaultPreload: 'intent'setting on TanStack Router will opportunistically preload lazy chunks on hover/focus, which is the right pairing for this pattern. The singlevendormanualChunkis a deliberate, documented trade-off to avoid a React 19 cross-chunk initialization bug; while it does mean editor-only dependencies like@use-gesture/reactship in the eagerly-loaded vendor bundle, the overall first-paint bytes still drop from 578 KB to ~460 KB on cold cache, with repeat visits dropping to just the 39 KB app chunk. No efficiency regressions, unbounded growth, blocking hot-path calls, or N+1 patterns are introduced by this change.🧯 Error handling & edge cases — No material issues found
No material issues found.
opencode/glm-5.2:cloud (opencode) — Minor issues
Verdict: Minor issues · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
I'll review this through the Security lens. The change is a frontend code-splitting refactor — let me verify the actual files.
VERDICT: No material issues found
Through the Security lens, this PR is a pure client-side bundle optimization with no security surface:
lazy(() => import('@/pages/...'))calls use static, hardcoded module specifiers — no user-controlled input reaches the dynamic import. Vite resolves these at build time into hashed chunk filenames; there is no runtimeimport(userInput)path. ThemanualChunkscallback returns a constant'vendor'string orundefined, never a value derived from request data.web/src/router.tsxin full: every guarded route still runs itsbeforeLoad(requireAuth,requireGuest,requireAdmin) before the lazy component mounts. TherequireAdmincomment atrouter.tsx:70correctly notes the server'srequireAdminremains the authoritative boundary — client routing is convenience only, unchanged by lazy loading.vite.config.tsonly readsPANSY_PORTfor the dev proxy, unchanged by this diff.embed.FS; no new route handling or path traversal vector is introduced on the frontend side.🎯 Correctness — No material issues found
I verified the change by reading the full
router.tsx,AppShell.tsx,vite.config.ts, the SPA fallback handler (internal/api/spa.go), thewebdistembed, and the import graphs of every lazy page (grep for@/pages/...and@/editor/...). Key checks:GardenEditorPage,PublicGardenPage,PlantsPage,SettingsPage,RegisterPage) are confirmed named exports (export function X), so the.then((m) => ({ default: m.X }))shape is correct.from '@/pages/(...)returns only the lazyimport()calls inrouter.tsx), so nothing accidentally re-eager-loads a split route and defeats the split.spa.go:51-77) serves realassets/files (where the hashed split chunks land) directly with immutable cache and only falls back toindex.htmlfor non-assets/paths, so deep-linking to/gardens/42and then fetching/assets/GardenEditorPage-<hash>.jsworks against the embed.manualChunksreturning'vendor'for anynode_modulesid andundefinedotherwise is semantically correct:undefineddefers to Rollup's default chunking, which creates the per-lazy-import chunks. Cross-chunk init order for the@use-gesturedep (lives invendor, imported by the editor chunk) is handled by Rollup's automatic cross-chunk imports.defaultPreload: 'intent'+ lazycomponentis fine: TanStack triggers the dynamic import on hover, and the AppShell<Suspense>covers the navigate case.One minor, inherent-to-
React.lazynote (not a bug introduced here): if a split chunk fails to load over a flaky cell connection — exactly the scenario this PR optimizes for — the rejectedlazyfactory throws on render and is caught by the route error boundary →RouteError/"Try again".router.invalidate()re-runs guards/loaders but does not force a remount of the lazy component, so the stuck-rejected factory can keep throwing until a full route change. This is standardReact.lazybehavior, not something this diff broke, and it pre-dates lazy routes only in the sense that there were no lazy routes before. Mentioning only because it's adjacent to the PR's stated mobile-cell-data goal.VERDICT: No material issues found
🧹 Code cleanliness & maintainability — Minor issues
Both findings confirmed. Lines 34-36 are indeed
importstatements sitting below the lazyconstblock (lines 21-33), which breaks the contiguous import group. PlantsPage on line 27 is one-line while the four siblings are multi-line.VERDICT: Minor issues
web/src/router.tsx:34-36— The remaining side-effect imports (queryClient,safeRedirectPath,getLastGardenId) sit below the block ofconst X = lazy(...)declarations (lines 21-33), splitting what used to be one contiguous import group into two halves with code in between. A reader scanning the top of the file for imports now has to jump past five lazy-component definitions to find the rest. Most import-ordering lint configs (simple-import-sort,eslint-plugin-import'simport/first) would flag imports below non-import statements. Suggested fix: move lines 34-36 up to sit with the other imports (lines 1-14), keeping the lazyconstblock as the first code block.web/src/router.tsx:27— ThePlantsPagelazy is collapsed to one line while the four siblings are each multi-line. Trivial, but a one-linegit blame/review sweep reads easier if the five are formatted consistently (either all one-liners or all multi-line). Not worth a churn-only commit, but if the ordering fix above is taken, normalize these in the same pass.⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed through the performance lens:
web/src/router.tsx: Thelazy()+ dynamicimport()per heavy route is the canonical code-splitting win; the editor chunk (60 KB) no longer blocks the login/list first paint. EagerLoginPage/GardensPageavoid a fallback flash on the hottest entry paths.web/vite.config.ts: The singlevendormanualChunk for allnode_modulesis a deliberate, justified cacheability choice (421 KB rarely-changing chunk cached across deploys vs. 39 KB app churn). ThemanualChunks(id)returnsundefinedfor app/source modules, so dynamic-import chunks remain independent — verified the predicate won't accidentally coalesce lazy route code intovendor(those module ids are app files undersrc/, notnode_modules).defaultPreload: 'intent'(router.tsx:180) still works with the lazy components — TanStack Router's intent preload triggers the dynamic import on hover/focus, so the chunk is typically warm by the time the user navigates. No regression, arguably better than before.AppShell.tsx:<Suspense>around<Outlet>only renders the fallback during the brief chunk fetch; no per-render overhead or re-creation of the boundary on normal navigation.No N+1, unbounded growth, hot-loop work, or avoidable quadratic behavior introduced.
🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run failed: exit status 1: [91m[1mError: [0mUnexpected error
database is locked
opencode/kimi-k2.6:cloud (opencode) — Minor issues
Verdict: Minor issues · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
Suspensefallback). All auth guards (beforeLoad) remain in the eagerly-loaded route definitions and execute before any lazy component chunk is fetched or rendered. No user input reaches animport()path; the lazy module specifiers are all static, compiler-resolved strings.🎯 Correctness — No material issues found
No material issues found.
React.lazywith.then(m => ({ default: m.NamedExport }))is the correct idiom for wrapping named exports. Verified all five page files use named exports (export function GardenEditorPage, etc.) and no default exports, so the wrapper is necessary and correct.<Outlet />inAppShellmeans eager routes (LoginPage, GardensPage) render synchronously through Suspense with no effect, while lazy routes suspend and show the fallback. This is the standard and correct pattern.internal/api/spa.goalready handles hashed assets underassets/withimmutablecaching and returns 404 for missing assets (preventing HTML fallback for broken JS requests). Code-split chunks will be served correctly from the embeddeddist/tree.vendorchunk is correctly documented. The comment accurately reflects the known React 19 initialization-order issue with splitting react-dom/scheduler into separate chunks.🧹 Code cleanliness & maintainability — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run returned no output
⚡ Performance — No material issues found
Verdict: No material issues found
This PR is a clean, well-reasoned code-splitting change. Through the performance lens:
vendormanualChunk is a defensible trade-off. Yes, a new visitor to/logindownloads 421 KB of vendor upfront (including@use-gesture/reactwhich only the editor uses), but this is both smaller than the old 578 KB monolith and cacheable across deploys. The PR author explicitly tried finer splitting and backed out because it triggers a known React 19 module-init ordering bug — the comment accurately records why.defaultPreload: 'intent'still applies. Hovering links to lazy routes will start fetching their chunks before the click, mitigating transition latency. No blocking work was added to hot paths.No performance findings to report.
🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues found
lazy()promise mapper hard-codes a named export (m.GardenEditorPage, etc.). If a page file ever changes to a default export or a barrel re-export,m.GardenEditorPagebecomesundefined; the promise still resolves, React.lazy accepts it, and the runtime throw is an opaque "Element type is invalid" rather than a clear chunk-load error. More importantly, there is no error boundary for a lazy chunk that fails to load or render. TanStack Router'serrorComponent(the existingRouteError) only catchesbeforeLoad/loadererrors; it does not catch React render errors or a rejectedimport()promise. A rejected chunk download (network drop, 404 on a hashed chunk after a deploy, server restart) will bubble pastSuspensewith no boundary to show a retry UI. Verified by readingRouteError.tsx:5-8and confirming it is only wired aserrorComponenton the root route, not a React<ErrorBoundary>.Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.