|
| 1 | +# TanStack Start Best-Practices Review |
| 2 | + |
| 3 | +Review of the whole storefront against the [tanstack-start best-practices skill](https://github.com/DeckardGer/tanstack-agent-skills/blob/main/skills/tanstack-start/SKILL.md) (all 13 rule files were fetched and used as the reference). Installed `@tanstack/react-start` version at review time: **1.168.34**. |
| 4 | + |
| 5 | +**Overall:** the app follows the skill's core patterns unusually well — server functions with Zod validation everywhere, `beforeLoad` route protection, httpOnly auth cookies, a GraphQL operation allowlist, `.functions.ts` / `.server.ts` file separation enforced by an architecture test, and deferred streaming on the product page. The findings below are the gaps, ordered by severity. |
| 6 | + |
| 7 | +--- |
| 8 | + |
| 9 | +## High |
| 10 | + |
| 11 | +### H1. Server-only env vars read in isomorphic code — client gets wrong `SITE_NAME` / `SITE_URL` |
| 12 | + |
| 13 | +**Rule:** `env-functions`, `ssr-hydration-safety` |
| 14 | +**Where:** `src/config/metadata.ts:3-4` |
| 15 | + |
| 16 | +```ts |
| 17 | +export const SITE_NAME = process.env.SITE_NAME || process.env.NEXT_PUBLIC_SITE_NAME || 'Vendure Store'; |
| 18 | +export const SITE_URL = process.env.SITE_URL || process.env.NEXT_PUBLIC_SITE_URL || 'https://example.com'; |
| 19 | +``` |
| 20 | + |
| 21 | +`config/metadata.ts` is imported by `src/platform/tanstack/head.ts` and `src/routes/__root.tsx`, and route `head()` functions execute on **both** server and client (on every client-side navigation). In the client bundle `process.env` is not populated with server env, so when `SITE_NAME`/`SITE_URL` are configured: |
| 22 | + |
| 23 | +- Server-rendered title/canonical/og tags use the configured values. |
| 24 | +- After hydration / on client navigation, head tags are recomputed with the fallbacks (`Vendure Store`, `https://example.com`). |
| 25 | + |
| 26 | +The e2e suite runs against `npm run dev` without these vars set, so this divergence is never exercised. |
| 27 | + |
| 28 | +**Fix options:** |
| 29 | +- Use Vite public env (`VITE_SITE_NAME` via `import.meta.env`) for values needed in head functions, or |
| 30 | +- Load them once server-side (root loader / router context) and read them from loader data in `head()`. |
| 31 | +- Drop the `NEXT_PUBLIC_*` fallbacks once the compatibility window ends — they are Next.js leftovers. |
| 32 | + |
| 33 | +--- |
| 34 | + |
| 35 | +## Medium |
| 36 | + |
| 37 | +### M1. Raw internal error messages are shown to users |
| 38 | + |
| 39 | +**Rule:** `err-server-errors` ("log full errors server-side, sanitize for client") |
| 40 | +**Where:** |
| 41 | +- `src/routes/__root.tsx:72` — `StorefrontError` renders `{error.message}` directly. |
| 42 | +- `src/platform/vendure/api.server.ts` — throws `Error(result.errors.map(e => e.message).join(', '))`, `'VENDURE_SHOP_API_URL environment variable is not set'`, `HTTP error! status: ...`. |
| 43 | +- `src/features/checkout/routes/actions.ts` — throws errors embedding Vendure `errorCode` and `message` (e.g. `Failed to transition order state: ${error.errorCode} - ${error.message}`). |
| 44 | + |
| 45 | +Server-function errors serialize across the network boundary, so backend GraphQL error text and configuration errors end up rendered in the root error component. The auth actions do this right (catch → return translated, generic `{ error }`); loaders and checkout actions do not. |
| 46 | + |
| 47 | +**Fix:** render a generic translated message in `StorefrontError` (log `error` instead of displaying it), and/or wrap `executeVendureRequest` failures in a sanitized error type before they leave the server. |
| 48 | + |
| 49 | +### M2. No middleware — cross-cutting concerns hand-repeated in every server function |
| 50 | + |
| 51 | +**Rule:** `mw-request-middleware`, `mw-function-middleware`, `sec-auth-middleware` |
| 52 | +**Where:** all `*.functions.ts` / `actions.ts` files; `createMiddleware` appears nowhere in `src/`. |
| 53 | + |
| 54 | +- `disableAuthResponseCaching()` is manually called at the top of 15 handlers; forgetting it in a new personalized function is a silent cache-safety bug. |
| 55 | +- Auth gating is repeated ad hoc: `if (!getAuthToken()) throw redirect({ to: "/sign-in" })` (`src/features/orders/order.functions.ts:35,55`), `if (!getAuthToken()) return null` (`auth.functions.ts:16`), plus the `authenticatedOperations` backstop in `api.server.ts`. |
| 56 | +- Locale + currency resolution (`getLocale()` + `await getActiveCurrencyCodeOnServer()`) is re-derived in nearly every handler. |
| 57 | + |
| 58 | +**Fix:** create composable server-function middleware, e.g. `noStoreMiddleware`, `authRequiredMiddleware` (throws redirect, passes token via context), and a `storefrontContextMiddleware` providing `{ locale, currencyCode }`. This is exactly the skill's canonical middleware use case. |
| 59 | + |
| 60 | +### M3. Root loader blocks every page render and disables all HTTP caching |
| 61 | + |
| 62 | +**Rule:** `ssr-prerender` / caching, `ssr-streaming` |
| 63 | +**Where:** `src/routes/__root.tsx:24-28`, `src/site/shell.functions.ts` |
| 64 | + |
| 65 | +`getShellData` runs in the root loader for every SSR request: 5 Vendure calls (channel, collections, active customer, active order, currency), two of them authenticated, and it sets `Cache-Control: no-store` — so **every HTML response in the app is uncacheable** (CDN and browser), and TTFB for public catalog pages is coupled to the slowest of those calls. The 30s `staleTime` only helps client-side navigations; the in-process `cachedPublicData` only covers the collections call. There is also no prerender config — every route is per-request SSR. |
| 66 | + |
| 67 | +This is a deliberate, documented trade-off (comment at `__root.tsx:25-27`), but it caps performance: |
| 68 | + |
| 69 | +- **Fix (incremental):** defer the personalized part — return cart count / customer name as an unawaited promise (streamed, rendered behind `<Await>`), keeping the public shell synchronous and cacheable. |
| 70 | +- **Fix (bigger):** fetch personalization client-side after hydration (or in a layout below root), then public pages can send `Cache-Control: public, s-maxage=...` and product/collection pages become CDN/ISR-cacheable per the skill's hybrid pattern. |
| 71 | + |
| 72 | +### M4. Duplicate Vendure requests within a single server request |
| 73 | + |
| 74 | +**Rule:** `mw-context-flow` (derive once, pass via context) |
| 75 | +**Where:** `src/features/currency/active-currency.server.ts:8`, `src/site/shell.functions.ts:18` |
| 76 | + |
| 77 | +For visitors without a currency cookie, `getActiveCurrencyCodeOnServer()` issues `GetActiveChannelQuery`. `getShellData` fires that same query in parallel with `getActiveCurrencyCodeOnServer()`, so first-time visitors trigger two identical channel queries per request; `getCheckoutRouteData` and every catalog function repeat the lookup again in their own request cycles. There is no request-scoped memoization. |
| 78 | + |
| 79 | +**Fix:** request-scoped cache (e.g. `AsyncLocalStorage`/context via middleware, or reuse the shell's channel result) so channel/currency is resolved once per request. |
| 80 | + |
| 81 | +### M5. No startup validation of environment configuration |
| 82 | + |
| 83 | +**Rule:** `env-functions` |
| 84 | +**Where:** `process.env` read ad hoc in `src/platform/vendure/api.server.ts:53-58`, `src/platform/revalidation/handler.ts:25`, `src/platform/vendure/auth-token.server.ts:4`, `src/config/metadata.ts:3-4` |
| 85 | + |
| 86 | +Required vars fail at request time, not boot: a missing `VENDURE_SHOP_API_URL` throws on the first page view (message shown to the user, see M1); a missing `REVALIDATION_SECRET` returns 500s from the webhook endpoint. A misconfigured `SITE_URL` silently emits `https://example.com` canonicals into production SEO tags. |
| 87 | + |
| 88 | +**Fix:** a single `env.server.ts` with a Zod schema validated once at startup (fail fast), imported everywhere else. |
| 89 | + |
| 90 | +### M6. No rate limiting on authentication endpoints |
| 91 | + |
| 92 | +**Rule:** `sf-input-validation` context ("consider rate limiting for mutation endpoints"), `sec-*` |
| 93 | +**Where:** `loginAction`, `registerAction`, `forgot-password` / `reset-password` actions |
| 94 | + |
| 95 | +Every `createServerFn` is a publicly reachable HTTP endpoint. Login and password-reset functions have no throttling, making credential stuffing / reset-email flooding cheap. Vendure applies its own protections upstream, and this is often solved at the edge — but the starter itself ships nothing and doesn't document the expectation. |
| 96 | + |
| 97 | +**Fix:** add rate-limit middleware (per-IP) for auth mutations, or document that deployments must provide it at the proxy/CDN layer. |
| 98 | + |
| 99 | +--- |
| 100 | + |
| 101 | +## Low |
| 102 | + |
| 103 | +### L1. Dead, publicly exposed server function |
| 104 | + |
| 105 | +**Where:** `src/features/currency/currency-server.ts` |
| 106 | + |
| 107 | +`getActiveCurrencyCode` is defined but never imported anywhere. Every `createServerFn` becomes a live RPC endpoint in the build, so unused ones are pure attack/maintenance surface. Remove it (and see L2 for the file naming). |
| 108 | + |
| 109 | +### L2. Currency module naming is confusing and off-convention |
| 110 | + |
| 111 | +**Rule:** `file-separation` |
| 112 | +**Where:** `src/features/currency/` — `active-currency.server.ts`, `currency.server.ts`, `currency-server.ts` |
| 113 | + |
| 114 | +Three near-identically named files with three different roles. `currency-server.ts` contains a server *function* but follows neither the `.functions.ts` convention (used by 20+ other files) nor `.server.ts`. `switch-currency.ts` also holds a server function without the `.functions.ts` suffix (as does `products/add-to-cart.ts` and `authentication/logout.ts`). Consistency here is what makes the boundary tests and the convention trustworthy. |
| 115 | + |
| 116 | +### L3. Login form duplicates the shared validation schema |
| 117 | + |
| 118 | +**Rule:** `file-shared-validation` |
| 119 | +**Where:** `src/features/authentication/routes/sign-in/login-form.tsx:22` vs `src/features/authentication/schemas.ts` |
| 120 | + |
| 121 | +The form defines a local `loginSchema` while the server action validates with the shared `loginInputSchema`. The duplicates can drift (they already differ: `.trim()` on the server only), and the client copy hardcodes English error strings while the rest of the app uses paraglide messages. Reuse `loginInputSchema` (minus `redirectTo`) with i18n error maps. The registration/reset forms are worth the same check. |
| 122 | + |
| 123 | +### L4. Auth cookie has no `maxAge` |
| 124 | + |
| 125 | +**Rule:** `auth-session-management` |
| 126 | +**Where:** `src/platform/vendure/auth-token.server.ts` (`setAuthToken`) |
| 127 | + |
| 128 | +`httpOnly`, `sameSite: 'lax'`, `secure` in production are all correct. But without `maxAge`/`expires` it is a browser-session cookie: customers are logged out (and lose guest carts) whenever the browser fully closes, even though the Vendure session token is longer-lived. If that's intentional, document it; otherwise set a `maxAge` aligned with Vendure's session duration. |
| 129 | + |
| 130 | +### L5. `/account` guard issues an uncached RPC on every navigation |
| 131 | + |
| 132 | +**Rule:** `auth-route-protection` (pattern is correct; cost is the issue) |
| 133 | +**Where:** `src/routes/account.tsx` `beforeLoad` |
| 134 | + |
| 135 | +`beforeLoad` runs on every navigation within the `/account` tree, and `getAccountSession` does a full `GetActiveCustomerQuery` round trip each time (it doesn't respect `staleTime`). Fine at this scale; consider caching the session in router context or memoizing per-request if account grows. |
| 136 | + |
| 137 | +### L6. Revalidation token compared with `!==` |
| 138 | + |
| 139 | +**Where:** `src/platform/revalidation/handler.ts:27` |
| 140 | + |
| 141 | +The bearer-secret check uses plain string comparison, which is theoretically timing-observable. Use `crypto.timingSafeEqual` on equal-length buffers. (Nit — the endpoint is otherwise well built: required secret, tag allowlist, per-request cap, structured 207 results.) |
| 142 | + |
| 143 | +### L7. Unused Next.js leftovers |
| 144 | + |
| 145 | +**Where:** `src/config/metadata.ts` (`buildCanonicalUrl`, `buildOgImages`, `noIndexRobots`), `src/platform/tanstack/metadata.ts` (`Metadata`, `Viewport` types), `NEXT_PUBLIC_*` env fallbacks |
| 146 | + |
| 147 | +These mirror the Next.js Metadata API and are referenced nowhere. Dead weight in a TanStack Start codebase; remove once the stated one-release compatibility window closes. |
| 148 | + |
| 149 | +### L8. TanStack packages pinned to `latest` |
| 150 | + |
| 151 | +**Where:** `package.json` — `@tanstack/react-router`, `@tanstack/react-start`, devtools packages |
| 152 | + |
| 153 | +`"latest"` makes builds non-reproducible and lets breaking upstream releases land silently (Start is still moving fast — e.g. the `inputValidator` → `validator` rename). The lockfile protects local installs but not fresh clones running `npm update` or template consumers. Pin to a caret range. |
| 154 | + |
| 155 | +--- |
| 156 | + |
| 157 | +## What the app gets right (verified against the rules) |
| 158 | + |
| 159 | +| Rule | Status | Evidence | |
| 160 | +|------|--------|----------| |
| 161 | +| `sf-create-server-fn` | ✅ | All data loading/mutations go through `createServerFn`; loaders call server functions, no client `fetch` to internal endpoints. | |
| 162 | +| `sf-input-validation` | ✅ | Every input-taking server function chains `.validator(zod schema)` (current API — `inputValidator` is the deprecated name in 1.168). Checked all 23 files defining server functions. | |
| 163 | +| `sf-method-selection` | ✅ | Reads are GET, mutations are POST throughout. | |
| 164 | +| `sec-validate-inputs` | ✅ | Search params validated via `validateSearch` + Zod (`catalogSearchSchema`, `redirectSearchSchema`, `tokenSearchSchema`). | |
| 165 | +| `sec-sensitive-data` | ✅ | GraphQL operation allowlist in `api.server.ts` blocks attacker-crafted selection sets — beyond what the skill asks for. `search.functions.ts` deliberately strips the raw result to avoid leaking session tokens. | |
| 166 | +| Open-redirect protection | ✅ | `safeInternalRedirect` applied both at the search-schema layer and inside `loginAction`. | |
| 167 | +| `auth-route-protection` | ✅ | `/account` uses `beforeLoad` + `redirect` with `redirectTo` preservation; context extended with `customer`; `noindex` on protected pages. | |
| 168 | +| `auth-cookie-security` | ✅ | httpOnly / sameSite lax / secure-in-prod (see L4 for `maxAge`). Token rotation from Vendure response headers is handled, including the tricky same-request rotation in checkout. | |
| 169 | +| `api-routes` | ✅ | `/api/revalidate` is a proper server-route handler (`server.handlers.POST`), not a misused server function. | |
| 170 | +| `ssr-streaming` | ✅ | Product page defers `relatedProducts` as an unawaited loader promise rendered via `<Await>`. | |
| 171 | +| `ssr-hydration-safety` | ✅ | Theme handled via `ScriptOnce` + `suppressHydrationWarning`; `window`/`matchMedia`/`localStorage` only inside effects; no `Date.now()`/`Math.random()` in render paths. (Exception: H1.) | |
| 172 | +| `err-not-found` | ✅ | `notFound()` thrown from loaders for missing products/collections; root `notFoundComponent` provided. | |
| 173 | +| `err-redirects` | ✅ | Server functions throw `redirect()` for auth/checkout-state flow control (`logoutAction`, `getCheckoutRouteData`, `placeOrder`), with `isRedirect` re-thrown in catch blocks. | |
| 174 | +| `file-separation` / `file-functions-file` | ✅ | `.server.ts` / `.functions.ts` conventions in place and **enforced by `tests/architecture/boundaries.test.mjs`** — stronger than the skill requires (see L2 for stragglers). | |
| 175 | +| Devtools in production | ✅ | `@tanstack/devtools-vite` plugin removes devtools code on build (`removeDevtoolsOnBuild` defaults to true) — the unconditional `<TanStackDevtools>` in `__root.tsx` is safe. | |
| 176 | +| Loader caching | ✅ | Sensible `staleTime` on root/catalog routes with documented invalidation via `router.invalidate()` after mutations. | |
| 177 | +| `pendingComponent` | ✅ | Provided on all slow routes (cart, checkout, product, collection, search). | |
| 178 | + |
| 179 | +--- |
| 180 | + |
| 181 | +## Suggested priority |
| 182 | + |
| 183 | +1. **H1** (config/env in isomorphic code) — silently breaks SEO metadata for any configured deployment. |
| 184 | +2. **M1** (error message leakage) — user-facing and trivial to hit (stop a Vendure instance and watch the message). |
| 185 | +3. **M2 + M4** (middleware + request-scoped context) — one refactor solves both and shrinks every server function. |
| 186 | +4. **M5** (startup env validation) — small, high leverage. |
| 187 | +5. **M3** — biggest performance win, but an architectural decision for the template. |
| 188 | +6. Low findings opportunistically. |
0 commit comments