-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat: (start) add option to load assets from different domain than the app #5545
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds a complete e2e test application for React Start with custom manifest base path support at Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as Express Server
participant Vite
participant SSR as SSR Handler
participant CDN as CDN Router
Note over Client,CDN: Development Mode
Client->>Server: GET /path
Server->>Vite: Load Vite middleware
Vite->>SSR: Load server entry via SSR
SSR->>Vite: Get client entry + assets
Vite->>Client: Render HTML with dev assets
Note over Client,CDN: Production Mode
Client->>CDN: GET /static/asset.js
CDN-->>Client: Serve from dist/client
Client->>Server: GET /api/path
Server->>SSR: Load prebuilt handler
SSR->>Client: Render HTML with CDN URLs
sequenceDiagram
participant Manifest as Route Manifest
participant Assets as Asset Rewriter
participant Output as Client Output
Note over Manifest,Output: Without assetsUrl
Manifest->>Assets: Process route preloads
Assets->>Output: Keep original URLs
Note over Manifest,Output: With assetsUrl
Manifest->>Assets: Process route preloads
Assets->>Assets: Prefix with assetsUrl
Assets->>Assets: Strip routerBasePath
Assets->>Output: Return rewritten URLs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Rationale: While the changes span 40+ files, the majority are new test app setup files following standard patterns (routes, components, utilities, configuration). Core framework changes are concentrated in three files ( Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@schiller-manuel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
🧹 Nitpick comments (23)
e2e/react-start/custom-manifest-base/public/site.webmanifest (1)
2-3: Consider adding placeholder names for test clarity.The
nameandshort_namefields are empty. While this might be intentional for a test fixture, adding placeholder values would make the manifest more realistic and easier to identify in browser dev tools during e2e testing.Apply this diff to add placeholder names:
{ - "name": "", - "short_name": "", + "name": "E2E Test App - Custom Manifest Base", + "short_name": "E2E Test", "icons": [e2e/react-start/custom-manifest-base/src/components/CustomMessage.tsx (1)
1-1: Remove unnecessary React import for modern JSX transform.React 19 uses the modern JSX transform by default, which doesn't require
Reactto be in scope. The import on Line 1 is unnecessary and can be safely removed.Apply this diff:
-import * as React from 'react' - export function CustomMessage({ message }: { message: string }) {e2e/react-start/custom-manifest-base/src/utils/users.tsx (1)
1-9: Use.tsextension instead of.tsxfor non-JSX files.This file contains only type definitions and constants without any JSX, so it should use the
.tsextension instead of.tsx. This follows TypeScript conventions and can slightly improve build performance.Rename the file:
e2e/react-start/custom-manifest-base/src/utils/users.tsx → e2e/react-start/custom-manifest-base/src/utils/users.tsThen update any imports in other files accordingly.
e2e/react-start/custom-manifest-base/src/components/NotFound.tsx (1)
1-25: Consider consolidating duplicated code across e2e tests.This NotFound component is identical to implementations in
e2e/react-start/basic-cloudflare,e2e/react-start/server-routes, and example apps. While duplication in e2e tests is acceptable, consider extracting to a shared test utilities package.Additionally, the
childrenprop usesany-React.ReactNodewould provide better type safety.-export function NotFound({ children }: { children?: any }) { +export function NotFound({ children }: { children?: React.ReactNode }) {e2e/react-start/custom-manifest-base/src/styles/app.css (1)
10-12: Scope globals and preserve accessible focus styles
- The universal border color apply can be heavy; consider scoping to elements that actually render borders or to
*, ::before, ::afterto avoid surprises.- Removing outlines under
.using-mouseis fine, but ensure visible:focus-visiblestyles exist for keyboard users.Also applies to: 19-21
packages/start-server-core/src/createStartHandler.ts (2)
56-57: Define and reuse a named options typeReplace the inline
{ assetsUrl?: string }with an exportedCreateStartHandlerOptionstype to keep API surface consistent across packages.-export function createStartHandler<TRegister = Register>( - cb: HandlerCallback<AnyRouter>, - opts: {assetsUrl?: string} = {} -): RequestHandler<TRegister> { +export type CreateStartHandlerOptions = { assetsUrl?: string } + +export function createStartHandler<TRegister = Register>( + cb: HandlerCallback<AnyRouter>, + opts: CreateStartHandlerOptions = {}, +): RequestHandler<TRegister> {
58-59: Normalize and validate assetsUrl before manifest rewritingGuard against double slashes and unsafe schemes, and avoid passing empty strings.
const ROUTER_BASEPATH = process.env.TSS_ROUTER_BASEPATH || '/' + const normalizeAssetsUrl = (raw?: string) => { + if (!raw) return undefined + try { + const u = new URL(raw) + if (u.protocol !== 'http:' && u.protocol !== 'https:') return undefined + // trim trailing slashes for stable join + u.pathname = u.pathname.replace(/\/+$/, '') + return u.toString().replace(/\/+$/, '') + } catch { + return undefined + } + }- if (startRoutesManifest === null) { - startRoutesManifest = await getStartManifest({ - ...opts, - routerBasePath: ROUTER_BASEPATH - }) - } + if (startRoutesManifest === null) { + startRoutesManifest = await getStartManifest({ + assetsUrl: normalizeAssetsUrl(opts.assetsUrl), + routerBasePath: ROUTER_BASEPATH, + }) + }Also verify in
router-manifest.tsthat link assethrefrewriting mirrorspreloadsbasepath stripping to avoid mixed paths. If not, align both.Also applies to: 216-219
e2e/react-start/custom-manifest-base/src/server.ts (1)
13-17: Make assetsUrl configurable and normalized (avoid hard‑coded localhost)Read from env for flexibility and trim trailing slashes to prevent
//when joining.-const fetch = createStartHandler(customHandler, {assetsUrl: 'http://localhost:3001'}) +const normalize = (raw?: string) => { + if (!raw) return undefined + try { + const u = new URL(raw) + u.pathname = u.pathname.replace(/\/+$/, '') + return u.toString().replace(/\/+$/, '') + } catch { + return undefined + } +} + +const fetch = createStartHandler(customHandler, { + assetsUrl: normalize(process.env.ASSETS_URL ?? 'http://localhost:3001'), +})Note: Prefer HTTPS in production to avoid mixed‑content/CORS issues.
packages/start-plugin-core/src/schema.ts (1)
132-132: Consider adding URL validation for the assetsUrl field.The
assetsUrlfield accepts any string without validating URL format. Since this configures asset loading from external domains, consider adding Zod URL validation to catch configuration errors early and prevent runtime issues.Apply this diff to add URL validation:
- assetsUrl: z.string().optional(), + assetsUrl: z.string().url().optional(),Additionally, consider documenting this field's purpose and security implications (CORS requirements, CSP policies) either in JSDoc comments or in user-facing documentation.
e2e/react-start/custom-manifest-base/src/routes/api.users.ts (1)
16-24: Add error handling for the external API call.The
axios.getcall on line 18 lacks error handling. If the external API fails or times out, the error will propagate as an unhandled exception, potentially breaking e2e tests.Apply this diff to add error handling:
GET: async ({ request }) => { console.info('Fetching users... @', request.url) - const res = await axios.get<Array<User>>(`${queryURL}/users`) - - const list = res.data.slice(0, 10) - - return json( - list.map((u) => ({ id: u.id, name: u.name, email: u.email })), - ) + try { + const res = await axios.get<Array<User>>(`${queryURL}/users`) + const list = res.data.slice(0, 10) + return json( + list.map((u) => ({ id: u.id, name: u.name, email: u.email })), + ) + } catch (error) { + console.error('Failed to fetch users:', error) + return json({ error: 'Failed to fetch users' }, { status: 500 }) + } },e2e/react-start/custom-manifest-base/src/routes/posts.tsx (1)
35-35: Consider adding ellipsis to truncated titles for better UX.The title is truncated to 20 characters without an ellipsis, which might make it unclear to users that the title is incomplete.
Apply this diff to add ellipsis:
- <div>{post.title.substring(0, 20)}</div> + <div> + {post.title.length > 20 + ? `${post.title.substring(0, 20)}...` + : post.title} + </div>e2e/react-start/custom-manifest-base/src/routes/posts.$postId.tsx (1)
16-18: Consider removing the PostErrorComponent wrapper.The
PostErrorComponentis a simple passthrough wrapper aroundErrorComponent. Unless you plan to add post-specific error handling logic later, you could directly useErrorComponentin the route configuration.Apply this diff to simplify:
- errorComponent: PostErrorComponent, + errorComponent: ErrorComponent,And remove the wrapper function:
-export function PostErrorComponent({ error }: ErrorComponentProps) { - return <ErrorComponent error={error} /> -} -e2e/react-start/custom-manifest-base/src/routes/users.$userId.tsx (2)
9-16: Preserve error details in the loader's error handling.The catch block on line 13 replaces all errors with a generic "Failed to fetch user" message, losing valuable debugging information such as whether it was a 404, 500, or network error. This makes troubleshooting e2e test failures more difficult.
Apply this diff to preserve error information:
loader: async ({ params: { userId } }) => { return await axios .get<User>('/api/users/' + userId) .then((r) => r.data) - .catch(() => { - throw new Error('Failed to fetch user') + .catch((error) => { + console.error('Failed to fetch user:', error) + throw new Error(`Failed to fetch user: ${error.message || 'Unknown error'}`) }) },
24-26: Consider removing the UserErrorComponent wrapper.Similar to
PostErrorComponentin posts.$postId.tsx, this is a passthrough wrapper. Unless you plan to add user-specific error handling, you could useErrorComponentdirectly.Apply this diff:
- errorComponent: UserErrorComponent, + errorComponent: ErrorComponent,And remove the wrapper:
-function UserErrorComponent({ error }: ErrorComponentProps) { - return <ErrorComponent error={error} /> -} -e2e/react-start/custom-manifest-base/src/routes/users.tsx (1)
6-13: Preserve error details in the loader's error handling.Similar to users.$userId.tsx, the catch block replaces all errors with a generic message, losing valuable debugging context. For e2e tests, preserving error details helps diagnose test failures.
Apply this diff:
loader: async () => { return await axios .get<Array<User>>('/api/users') .then((r) => r.data) - .catch(() => { - throw new Error('Failed to fetch users') + .catch((error) => { + console.error('Failed to fetch users:', error) + throw new Error(`Failed to fetch users: ${error.message || 'Unknown error'}`) }) },e2e/react-start/custom-manifest-base/src/utils/seo.ts (1)
12-30: Avoid meta tags with undefined contentOnly include description/keywords when defined to prevent
content="undefined"in markup.const tags = [ { title }, - { name: 'description', content: description }, - { name: 'keywords', content: keywords }, + ...(description ? [{ name: 'description', content: description }] : []), + ...(keywords ? [{ name: 'keywords', content: keywords }] : []), { name: 'twitter:title', content: title }, { name: 'twitter:description', content: description },e2e/react-start/custom-manifest-base/playwright.config.ts (1)
29-34: Make webServer env cross‑platform (Windows‑safe)Inline
FOO=bar pnpm ...breaks on Windows. Use Playwright’swebServer.envinstead.- webServer: { - command: `VITE_NODE_ENV="test" VITE_EXTERNAL_PORT=${EXTERNAL_PORT} pnpm build && VITE_NODE_ENV="test" VITE_EXTERNAL_PORT=${EXTERNAL_PORT} VITE_SERVER_PORT=${PORT} PORT=${PORT} pnpm start`, - url: baseURL, - reuseExistingServer: !process.env.CI, - stdout: 'pipe', - }, + webServer: { + command: `pnpm build && pnpm start`, + url: baseURL, + reuseExistingServer: !process.env.CI, + stdout: 'pipe', + env: { + VITE_NODE_ENV: 'test', + VITE_EXTERNAL_PORT: String(EXTERNAL_PORT), + VITE_SERVER_PORT: String(PORT), + PORT: String(PORT), + }, + },e2e/react-start/custom-manifest-base/tests/navigation.spec.ts (1)
63-69: Also assert redirect status codeStrengthen the server‑side redirect test by checking status (commonly 302/307).
await page.request .get('/redirect/throw-it', { maxRedirects: 0 }) .then((res) => { + expect([302, 303, 307, 308]).toContain(res.status()) const headers = new Headers(res.headers()) expect(headers.get('location')).toBe('/custom/basepath/posts/1') })e2e/react-start/custom-manifest-base/src/utils/posts.tsx (2)
17-19: Input validator is a pass‑throughValidate and normalize
postId(digits only) to avoid accidental invalid requests.export const fetchPost = createServerFn({ method: 'GET' }) - .inputValidator((postId: string) => postId) + .inputValidator((postId: unknown) => { + if (typeof postId !== 'string' || !/^\d+$/.test(postId)) { + throw new Error('postId must be a numeric string') + } + return postId + })
5-9: Align PostType.id with API (number)jsonplaceholder returns numeric
id. Considerid: numberto match data.e2e/react-start/custom-manifest-base/src/routes/__root.tsx (2)
85-86: Add language attribute for accessibility/SEOSet
<html lang="en">(or appropriate locale).- return ( - <html> + return ( + <html lang="en">
33-52: Make icon/manifest links basepath‑aware; fix invalid attributeAbsolute
href: '/favicon-32x32.png'can break under a basepath/CDN, andcoloronrel="manifest"is invalid (also value#fffffis not a valid hex). Prefer basepath‑prefixed hrefs and drop the stray attribute.links: [ { rel: 'apple-touch-icon', sizes: '180x180', - href: '/apple-touch-icon.png', + href: '/custom/basepath/apple-touch-icon.png', }, { rel: 'icon', type: 'image/png', sizes: '32x32', - href: '/favicon-32x32.png', + href: '/custom/basepath/favicon-32x32.png', }, { rel: 'icon', type: 'image/png', sizes: '16x16', - href: '/favicon-16x16.png', + href: '/custom/basepath/favicon-16x16.png', }, - { rel: 'manifest', href: '/site.webmanifest', color: '#fffff' }, - { rel: 'icon', href: '/favicon.ico' }, + { rel: 'manifest', href: '/custom/basepath/site.webmanifest' }, + { rel: 'icon', href: '/custom/basepath/favicon.ico' }, ],If a shared
basepathutil exists (e.g.,~/utils/basepath), import and interpolate instead of hardcoding.packages/start-server-core/src/router-manifest.ts (1)
59-72: Optional: also handle <script src> assets if presentIf
assetscan include<script src>, mirror the same rewriting forasset.tag === 'script' && asset.attrs?.src.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
e2e/react-start/custom-manifest-base/public/android-chrome-192x192.pngis excluded by!**/*.pnge2e/react-start/custom-manifest-base/public/android-chrome-512x512.pngis excluded by!**/*.pnge2e/react-start/custom-manifest-base/public/apple-touch-icon.pngis excluded by!**/*.pnge2e/react-start/custom-manifest-base/public/favicon-16x16.pngis excluded by!**/*.pnge2e/react-start/custom-manifest-base/public/favicon-32x32.pngis excluded by!**/*.pnge2e/react-start/custom-manifest-base/public/favicon.icois excluded by!**/*.icoe2e/react-start/custom-manifest-base/public/favicon.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (46)
e2e/react-start/custom-manifest-base/.gitignore(1 hunks)e2e/react-start/custom-manifest-base/.prettierignore(1 hunks)e2e/react-start/custom-manifest-base/express-server.ts(1 hunks)e2e/react-start/custom-manifest-base/package.json(1 hunks)e2e/react-start/custom-manifest-base/playwright.config.ts(1 hunks)e2e/react-start/custom-manifest-base/postcss.config.mjs(1 hunks)e2e/react-start/custom-manifest-base/public/script.js(1 hunks)e2e/react-start/custom-manifest-base/public/script2.js(1 hunks)e2e/react-start/custom-manifest-base/public/site.webmanifest(1 hunks)e2e/react-start/custom-manifest-base/src/components/CustomMessage.tsx(1 hunks)e2e/react-start/custom-manifest-base/src/components/DefaultCatchBoundary.tsx(1 hunks)e2e/react-start/custom-manifest-base/src/components/NotFound.tsx(1 hunks)e2e/react-start/custom-manifest-base/src/routeTree.gen.ts(1 hunks)e2e/react-start/custom-manifest-base/src/router.tsx(1 hunks)e2e/react-start/custom-manifest-base/src/routes/__root.tsx(1 hunks)e2e/react-start/custom-manifest-base/src/routes/api.users.ts(1 hunks)e2e/react-start/custom-manifest-base/src/routes/api/users.$id.ts(1 hunks)e2e/react-start/custom-manifest-base/src/routes/deferred.tsx(1 hunks)e2e/react-start/custom-manifest-base/src/routes/index.tsx(1 hunks)e2e/react-start/custom-manifest-base/src/routes/logout.tsx(1 hunks)e2e/react-start/custom-manifest-base/src/routes/posts.$postId.tsx(1 hunks)e2e/react-start/custom-manifest-base/src/routes/posts.index.tsx(1 hunks)e2e/react-start/custom-manifest-base/src/routes/posts.tsx(1 hunks)e2e/react-start/custom-manifest-base/src/routes/posts_.$postId.deep.tsx(1 hunks)e2e/react-start/custom-manifest-base/src/routes/redirect/index.tsx(1 hunks)e2e/react-start/custom-manifest-base/src/routes/redirect/throw-it.tsx(1 hunks)e2e/react-start/custom-manifest-base/src/routes/users.$userId.tsx(1 hunks)e2e/react-start/custom-manifest-base/src/routes/users.index.tsx(1 hunks)e2e/react-start/custom-manifest-base/src/routes/users.tsx(1 hunks)e2e/react-start/custom-manifest-base/src/server.ts(1 hunks)e2e/react-start/custom-manifest-base/src/styles/app.css(1 hunks)e2e/react-start/custom-manifest-base/src/utils/basepath.ts(1 hunks)e2e/react-start/custom-manifest-base/src/utils/posts.tsx(1 hunks)e2e/react-start/custom-manifest-base/src/utils/seo.ts(1 hunks)e2e/react-start/custom-manifest-base/src/utils/users.tsx(1 hunks)e2e/react-start/custom-manifest-base/tailwind.config.mjs(1 hunks)e2e/react-start/custom-manifest-base/tests/navigation.spec.ts(1 hunks)e2e/react-start/custom-manifest-base/tests/setup/global.setup.ts(1 hunks)e2e/react-start/custom-manifest-base/tests/setup/global.teardown.ts(1 hunks)e2e/react-start/custom-manifest-base/tsconfig.json(1 hunks)e2e/react-start/custom-manifest-base/vite.config.ts(1 hunks)package.json(2 hunks)packages/server-functions-plugin/src/index.ts(2 hunks)packages/start-plugin-core/src/schema.ts(1 hunks)packages/start-server-core/src/createStartHandler.ts(2 hunks)packages/start-server-core/src/router-manifest.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
e2e/react-start/custom-manifest-base/tests/navigation.spec.tse2e/react-start/custom-manifest-base/src/routes/redirect/throw-it.tsxe2e/react-start/custom-manifest-base/src/components/DefaultCatchBoundary.tsxe2e/react-start/custom-manifest-base/src/components/CustomMessage.tsxe2e/react-start/custom-manifest-base/src/components/NotFound.tsxe2e/react-start/custom-manifest-base/src/router.tsxe2e/react-start/custom-manifest-base/src/utils/seo.tse2e/react-start/custom-manifest-base/tests/setup/global.teardown.tse2e/react-start/custom-manifest-base/src/server.tse2e/react-start/custom-manifest-base/src/routes/posts_.$postId.deep.tsxe2e/react-start/custom-manifest-base/src/routes/posts.tsxe2e/react-start/custom-manifest-base/src/routes/redirect/index.tsxe2e/react-start/custom-manifest-base/src/routes/users.tsxe2e/react-start/custom-manifest-base/src/routes/users.$userId.tsxe2e/react-start/custom-manifest-base/src/utils/users.tsxpackages/server-functions-plugin/src/index.tspackages/start-server-core/src/createStartHandler.tse2e/react-start/custom-manifest-base/src/routes/posts.index.tsxpackages/start-server-core/src/router-manifest.tspackages/start-plugin-core/src/schema.tse2e/react-start/custom-manifest-base/src/routes/api.users.tse2e/react-start/custom-manifest-base/src/routes/posts.$postId.tsxe2e/react-start/custom-manifest-base/src/routes/index.tsxe2e/react-start/custom-manifest-base/src/utils/posts.tsxe2e/react-start/custom-manifest-base/express-server.tse2e/react-start/custom-manifest-base/src/utils/basepath.tse2e/react-start/custom-manifest-base/src/routes/api/users.$id.tse2e/react-start/custom-manifest-base/src/routes/logout.tsxe2e/react-start/custom-manifest-base/vite.config.tse2e/react-start/custom-manifest-base/src/routes/__root.tsxe2e/react-start/custom-manifest-base/src/routes/deferred.tsxe2e/react-start/custom-manifest-base/playwright.config.tse2e/react-start/custom-manifest-base/tests/setup/global.setup.tse2e/react-start/custom-manifest-base/src/routeTree.gen.tse2e/react-start/custom-manifest-base/src/routes/users.index.tsx
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/react-start/custom-manifest-base/tests/navigation.spec.tse2e/react-start/custom-manifest-base/src/routes/redirect/throw-it.tsxe2e/react-start/custom-manifest-base/src/components/DefaultCatchBoundary.tsxe2e/react-start/custom-manifest-base/src/components/CustomMessage.tsxe2e/react-start/custom-manifest-base/src/components/NotFound.tsxe2e/react-start/custom-manifest-base/postcss.config.mjse2e/react-start/custom-manifest-base/src/router.tsxe2e/react-start/custom-manifest-base/src/utils/seo.tse2e/react-start/custom-manifest-base/tests/setup/global.teardown.tse2e/react-start/custom-manifest-base/src/server.tse2e/react-start/custom-manifest-base/src/routes/posts_.$postId.deep.tsxe2e/react-start/custom-manifest-base/src/routes/posts.tsxe2e/react-start/custom-manifest-base/src/routes/redirect/index.tsxe2e/react-start/custom-manifest-base/package.jsone2e/react-start/custom-manifest-base/src/routes/users.tsxe2e/react-start/custom-manifest-base/src/routes/users.$userId.tsxe2e/react-start/custom-manifest-base/src/utils/users.tsxe2e/react-start/custom-manifest-base/src/styles/app.csse2e/react-start/custom-manifest-base/src/routes/posts.index.tsxe2e/react-start/custom-manifest-base/src/routes/api.users.tse2e/react-start/custom-manifest-base/src/routes/posts.$postId.tsxe2e/react-start/custom-manifest-base/src/routes/index.tsxe2e/react-start/custom-manifest-base/src/utils/posts.tsxe2e/react-start/custom-manifest-base/express-server.tse2e/react-start/custom-manifest-base/src/utils/basepath.tse2e/react-start/custom-manifest-base/public/script.jse2e/react-start/custom-manifest-base/public/site.webmanifeste2e/react-start/custom-manifest-base/public/script2.jse2e/react-start/custom-manifest-base/src/routes/api/users.$id.tse2e/react-start/custom-manifest-base/src/routes/logout.tsxe2e/react-start/custom-manifest-base/vite.config.tse2e/react-start/custom-manifest-base/tsconfig.jsone2e/react-start/custom-manifest-base/src/routes/__root.tsxe2e/react-start/custom-manifest-base/src/routes/deferred.tsxe2e/react-start/custom-manifest-base/tailwind.config.mjse2e/react-start/custom-manifest-base/playwright.config.tse2e/react-start/custom-manifest-base/tests/setup/global.setup.tse2e/react-start/custom-manifest-base/src/routeTree.gen.tse2e/react-start/custom-manifest-base/src/routes/users.index.tsx
**/src/routes/**
📄 CodeRabbit inference engine (AGENTS.md)
Place file-based routes under src/routes/ directories
Files:
e2e/react-start/custom-manifest-base/src/routes/redirect/throw-it.tsxe2e/react-start/custom-manifest-base/src/routes/posts_.$postId.deep.tsxe2e/react-start/custom-manifest-base/src/routes/posts.tsxe2e/react-start/custom-manifest-base/src/routes/redirect/index.tsxe2e/react-start/custom-manifest-base/src/routes/users.tsxe2e/react-start/custom-manifest-base/src/routes/users.$userId.tsxe2e/react-start/custom-manifest-base/src/routes/posts.index.tsxe2e/react-start/custom-manifest-base/src/routes/api.users.tse2e/react-start/custom-manifest-base/src/routes/posts.$postId.tsxe2e/react-start/custom-manifest-base/src/routes/index.tsxe2e/react-start/custom-manifest-base/src/routes/api/users.$id.tse2e/react-start/custom-manifest-base/src/routes/logout.tsxe2e/react-start/custom-manifest-base/src/routes/__root.tsxe2e/react-start/custom-manifest-base/src/routes/deferred.tsxe2e/react-start/custom-manifest-base/src/routes/users.index.tsx
**/package.json
📄 CodeRabbit inference engine (AGENTS.md)
Use workspace:* protocol for internal dependencies in package.json files
Files:
e2e/react-start/custom-manifest-base/package.jsonpackage.json
packages/{*-start,start-*}/**
📄 CodeRabbit inference engine (AGENTS.md)
Name and place Start framework packages under packages/-start/ or packages/start-/
Files:
packages/start-server-core/src/createStartHandler.tspackages/start-server-core/src/router-manifest.tspackages/start-plugin-core/src/schema.ts
🧠 Learnings (2)
📚 Learning: 2025-09-23T17:36:12.598Z
Learnt from: CR
PR: TanStack/router#0
File: AGENTS.md:0-0
Timestamp: 2025-09-23T17:36:12.598Z
Learning: Applies to **/*.{ts,tsx} : Use TypeScript in strict mode with extensive type safety across the codebase
Applied to files:
e2e/react-start/custom-manifest-base/tsconfig.json
📚 Learning: 2025-10-01T18:31:35.420Z
Learnt from: schiller-manuel
PR: TanStack/router#5330
File: e2e/react-start/custom-basepath/src/routeTree.gen.ts:58-61
Timestamp: 2025-10-01T18:31:35.420Z
Learning: Do not review files named `routeTree.gen.ts` in TanStack Router repositories, as these are autogenerated files that should not be manually modified.
Applied to files:
e2e/react-start/custom-manifest-base/src/routeTree.gen.ts
🧬 Code graph analysis (22)
e2e/react-start/custom-manifest-base/src/routes/redirect/throw-it.tsx (2)
e2e/react-start/custom-manifest-base/src/routes/posts.$postId.tsx (1)
Route(7-14)e2e/react-start/custom-manifest-base/src/routes/redirect/index.tsx (1)
Route(3-5)
e2e/react-start/custom-manifest-base/src/components/DefaultCatchBoundary.tsx (1)
e2e/react-start/server-routes/src/components/DefaultCatchBoundary.tsx (1)
DefaultCatchBoundary(10-53)
e2e/react-start/custom-manifest-base/src/components/NotFound.tsx (3)
e2e/react-start/basic-cloudflare/src/components/NotFound.tsx (1)
NotFound(3-25)e2e/react-start/server-routes/src/components/NotFound.tsx (1)
NotFound(3-25)examples/react/start-basic-cloudflare/src/components/NotFound.tsx (1)
NotFound(3-25)
e2e/react-start/custom-manifest-base/src/router.tsx (2)
e2e/react-start/custom-manifest-base/src/components/DefaultCatchBoundary.tsx (1)
DefaultCatchBoundary(10-53)e2e/react-start/custom-manifest-base/src/components/NotFound.tsx (1)
NotFound(3-25)
e2e/react-start/custom-manifest-base/src/utils/seo.ts (1)
examples/solid/start-basic-netlify/src/utils/seo.ts (1)
title(1-33)
e2e/react-start/custom-manifest-base/src/server.ts (2)
e2e/react-start/custom-basepath/src/server.ts (1)
fetch(4-6)packages/start-server-core/src/createStartHandler.ts (1)
createStartHandler(54-344)
e2e/react-start/custom-manifest-base/src/routes/posts.tsx (3)
e2e/react-start/custom-manifest-base/src/routes/posts.$postId.tsx (1)
Route(7-14)e2e/react-start/custom-manifest-base/src/routes/posts.index.tsx (1)
Route(2-4)e2e/react-start/custom-manifest-base/src/utils/posts.tsx (1)
fetchPosts(35-42)
e2e/react-start/custom-manifest-base/src/routes/redirect/index.tsx (1)
e2e/react-start/custom-manifest-base/src/routes/redirect/throw-it.tsx (1)
Route(3-10)
e2e/react-start/custom-manifest-base/src/routes/users.tsx (5)
e2e/react-start/custom-manifest-base/src/routes/api.users.ts (1)
Route(13-28)e2e/react-start/custom-manifest-base/src/routes/api/users.$id.ts (1)
Route(12-32)e2e/react-start/custom-manifest-base/src/routes/users.$userId.tsx (1)
Route(8-22)e2e/react-start/custom-manifest-base/src/utils/users.tsx (1)
User(1-5)examples/solid/start-basic/src/routes/users.tsx (1)
UsersComponent(18-48)
e2e/react-start/custom-manifest-base/src/routes/users.$userId.tsx (3)
e2e/react-start/custom-manifest-base/src/routes/api/users.$id.ts (1)
Route(12-32)e2e/react-start/custom-manifest-base/src/utils/users.tsx (1)
User(1-5)e2e/react-start/custom-manifest-base/src/components/NotFound.tsx (1)
NotFound(3-25)
packages/start-server-core/src/createStartHandler.ts (1)
packages/start-server-core/src/router-manifest.ts (1)
getStartManifest(11-81)
e2e/react-start/custom-manifest-base/src/routes/posts.index.tsx (3)
e2e/react-start/custom-manifest-base/src/routes/posts.$postId.tsx (1)
Route(7-14)e2e/react-start/custom-manifest-base/src/routes/posts.tsx (1)
Route(5-15)e2e/react-start/custom-manifest-base/src/routes/users.index.tsx (1)
Route(2-4)
e2e/react-start/custom-manifest-base/src/routes/api.users.ts (4)
e2e/react-start/custom-manifest-base/src/routes/api/users.$id.ts (1)
Route(12-32)e2e/react-start/custom-manifest-base/src/routes/users.$userId.tsx (1)
Route(8-22)e2e/react-start/custom-manifest-base/src/routes/users.tsx (1)
Route(5-15)e2e/react-start/custom-manifest-base/src/utils/users.tsx (1)
User(1-5)
e2e/react-start/custom-manifest-base/src/routes/posts.$postId.tsx (3)
e2e/react-start/custom-manifest-base/src/routes/posts_.$postId.deep.tsx (1)
Route(6-10)e2e/react-start/custom-manifest-base/src/utils/posts.tsx (1)
fetchPost(17-33)e2e/react-start/custom-manifest-base/src/components/NotFound.tsx (1)
NotFound(3-25)
e2e/react-start/custom-manifest-base/src/routes/index.tsx (1)
e2e/react-start/custom-manifest-base/src/components/CustomMessage.tsx (1)
CustomMessage(3-10)
e2e/react-start/custom-manifest-base/src/utils/posts.tsx (1)
e2e/react-router/js-only-file-based/src/posts.js (1)
queryURL(5-5)
e2e/react-start/custom-manifest-base/src/routes/api/users.$id.ts (4)
e2e/react-router/js-only-file-based/src/posts.js (1)
queryURL(5-5)e2e/react-start/custom-manifest-base/src/routes/api.users.ts (1)
Route(13-28)e2e/react-start/custom-manifest-base/src/routes/users.$userId.tsx (1)
Route(8-22)e2e/react-start/custom-manifest-base/src/utils/users.tsx (1)
User(1-5)
e2e/react-start/custom-manifest-base/src/routes/__root.tsx (4)
e2e/react-start/custom-manifest-base/src/utils/seo.ts (1)
seo(1-33)e2e/react-start/custom-manifest-base/src/components/DefaultCatchBoundary.tsx (1)
DefaultCatchBoundary(10-53)e2e/react-start/custom-manifest-base/src/components/NotFound.tsx (1)
NotFound(3-25)examples/react/router-monorepo-simple-lazy/packages/app/src/rootComponent.tsx (1)
RootComponent(4-24)
e2e/react-start/custom-manifest-base/playwright.config.ts (1)
scripts/set-ts-version.js (1)
packageJson(33-33)
e2e/react-start/custom-manifest-base/tests/setup/global.setup.ts (1)
scripts/set-ts-version.js (1)
packageJson(33-33)
e2e/react-start/custom-manifest-base/src/routeTree.gen.ts (1)
e2e/react-start/custom-manifest-base/src/router.tsx (1)
getRouter(6-16)
e2e/react-start/custom-manifest-base/src/routes/users.index.tsx (3)
e2e/react-start/custom-manifest-base/src/routes/api.users.ts (1)
Route(13-28)e2e/react-start/custom-manifest-base/src/routes/users.$userId.tsx (1)
Route(8-22)e2e/react-start/custom-manifest-base/src/routes/users.tsx (1)
Route(5-15)
🪛 Biome (2.1.2)
e2e/react-start/custom-manifest-base/src/routes/deferred.tsx
[error] 43-43: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
[error] 53-53: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
🔇 Additional comments (31)
e2e/react-start/custom-manifest-base/.gitignore (1)
1-20: Gitignore patterns look appropriate for a React Start test application.The file includes standard exclusions for Node.js dependencies, lock files, build outputs, deployment artifacts, and test results. The Sentry configuration pattern and build paths specific to the Start framework (e.g.,
/server/build,/public/build) are well-aligned with the test application's structure.You may want to verify that the build output paths (lines 11-14) match the actual output directories of your test application, but no changes are necessary from a general best-practices perspective.
packages/server-functions-plugin/src/index.ts (1)
207-209: Verify ifextractedFilenameis needed at runtime or only for debugging.The
extractedFilenamefield is added to the manifest and appears to be used only for logging at line 217. If this field is solely for debugging purposes, consider conditionally including it based on the existingdebugflag (lines 33-35) to reduce the manifest size in production builds.If
extractedFilenameis required for other purposes beyond debugging (e.g., error reporting, source mapping), please clarify the use case. Otherwise, apply this optimization:const mod = ` const manifest = {${Object.entries(directiveFnsById) .map( ([id, fn]: any) => \`'\${id}': { functionName: '\${fn.functionName}', - importer: () => import(\${JSON.stringify(fn.extractedFilename)}), - extractedFilename: \${JSON.stringify(fn.extractedFilename)} + importer: () => import(\${JSON.stringify(fn.extractedFilename)})\${debug ? \`,\nextractedFilename: \${JSON.stringify(fn.extractedFilename)}\` : ''} }\`, ) .join(',')}}e2e/react-start/custom-manifest-base/public/site.webmanifest (1)
4-15: The icon files referenced in the manifest are present in the repository ate2e/react-start/custom-manifest-base/public/. Bothandroid-chrome-192x192.pngandandroid-chrome-512x512.pngexist as expected, so there is no risk of 404 errors or PWA functionality issues related to missing icon files.e2e/react-start/custom-manifest-base/src/routes/api/users.$id.ts (2)
1-10: LGTM!The environment-aware URL configuration is correctly implemented using Vite's
import.meta.env, and the pattern is consistent with other routes in the application.
15-29: Verify error handling covers network failures appropriately.The catch block returns a 404 for all errors, including network failures and timeouts. While this is acceptable for an e2e test route, ensure that production implementations distinguish between "not found" (404) and other failure modes (e.g., 500 for upstream errors, 503 for timeouts).
e2e/react-start/custom-manifest-base/tsconfig.json (1)
1-23: LGTM!The TypeScript configuration correctly enables strict mode and uses modern settings appropriate for a Vite-based React application. The configuration aligns with the coding guidelines for strict TypeScript.
Based on learnings.
e2e/react-start/custom-manifest-base/express-server.ts (3)
1-11: LGTM!The server setup correctly initializes separate Express instances for the main app and CDN, with environment-aware configuration.
12-31: LGTM!The development mode correctly integrates Vite SSR with proper error handling and stack trace fixing. The dynamic loading of the server entry via
ssrLoadModuleis appropriate for development hot reloading.
35-35: CORS is wide open for the CDN.Line 35 enables CORS without restrictions, allowing any origin to access static assets. This is acceptable for an e2e test environment but should be restricted in production deployments.
e2e/react-start/custom-manifest-base/src/components/DefaultCatchBoundary.tsx (1)
1-53: LGTM!The error boundary implementation correctly handles error display and recovery actions. The root route detection and conditional navigation logic are properly implemented using TanStack Router hooks.
e2e/react-start/custom-manifest-base/src/routes/deferred.tsx (2)
5-16: LGTM!The server functions correctly demonstrate immediate and delayed responses for testing deferred loading behavior.
40-55: Biome warning is a false positive.The
childrenprop onAwaitcomponents (lines 43 and 53) is the correct API for TanStack Router'sAwaitcomponent. The BiomenoChildrenProprule doesn't recognize this as a valid pattern. The implementation is correct as written.e2e/react-start/custom-manifest-base/src/utils/basepath.ts (1)
1-1: LGTM!The basepath constant is correctly defined and aligns with the PR objective of testing custom manifest base paths.
e2e/react-start/custom-manifest-base/src/routes/redirect/index.tsx (1)
1-15: LGTM!The redirect route is correctly implemented with appropriate test annotations. It works in conjunction with the
/redirect/throw-itroute to test redirect functionality.package.json (1)
8-8: pnpm version bump verified as compatible.The update from
pnpm@10.13.1topnpm@10.18.3is a standard patch version bump with only bug fixes and no breaking changes. The release includes fixes for infinite recursion in install scripts, registry key parsing, CLI option handling, and bin link synchronization—all targeted improvements that should not impact build processes or dependencies.e2e/react-start/custom-manifest-base/src/routes/redirect/throw-it.tsx (1)
1-10: LGTM!The redirect implementation correctly uses
beforeLoadto throw a navigation redirect, which is the proper pattern for TanStack Router.e2e/react-start/custom-manifest-base/tailwind.config.mjs (1)
1-4: LGTM!Standard Tailwind configuration with appropriate content paths.
e2e/react-start/custom-manifest-base/src/routes/index.tsx (1)
1-15: LGTM!Clean home route implementation with proper component composition.
e2e/react-start/custom-manifest-base/postcss.config.mjs (1)
1-6: LGTM!Standard PostCSS configuration for Tailwind CSS with autoprefixer.
e2e/react-start/custom-manifest-base/tests/setup/global.teardown.ts (1)
1-6: LGTM!Proper test teardown implementation that cleanly stops the e2e server.
e2e/react-start/custom-manifest-base/src/routes/users.index.tsx (1)
1-8: LGTM!Simple and appropriate index route for the users section.
e2e/react-start/custom-manifest-base/src/router.tsx (1)
1-16: LGTM!Well-structured router configuration with appropriate defaults for error handling, preloading, and scroll restoration.
packages/start-server-core/src/createStartHandler.ts (1)
54-57: Signature extension looks backward‑compatibleOptional
optswith a default preserves existing call sites. LGTM.e2e/react-start/custom-manifest-base/src/routes/posts.index.tsx (1)
1-8: LGTM: minimal index route is clear and testableSimple component with data-testid fits the e2e use case.
e2e/react-start/custom-manifest-base/src/routes/posts_.$postId.deep.tsx (1)
6-10: Clarify route path: 'posts_' appears to be inconsistent with sibling routes.The route path
/posts_/$postId/deepin this file is the only one using theposts_prefix; all sibling routes use/posts/without underscore. No documentation explains this deviation. If intentional, add a comment clarifying why; if unintentional, rename to/posts/$postId/deepto match the convention.e2e/react-start/custom-manifest-base/tests/setup/global.setup.ts (1)
1-6: LGTM!The test setup correctly uses the modern import attributes syntax (
with { type: 'json' }) and properly initializes the e2e dummy server with the package name. The async/await pattern is correctly applied.e2e/react-start/custom-manifest-base/src/routes/logout.tsx (2)
4-11: LGTM!The server function correctly uses the
throw redirect()pattern for redirecting after logout, which is the idiomatic way to handle redirects in TanStack Start server functions.
17-31: LGTM!The logout form correctly demonstrates server function URL behavior with basepath. The hardcoded CSRF token is appropriate for e2e testing purposes. The form properly uses the POST method and references
logoutFn.urlfor the action.e2e/react-start/custom-manifest-base/playwright.config.ts (1)
6-6: Verify JSON import support in TS configEnsure
resolveJsonModule: trueandmoduleResolution: "NodeNext"(or equivalent) are enabled soimport ... with { type: 'json' }compiles in Playwright. Otherwise, switch tofs.readFileSync(...)/JSON.parse.packages/start-server-core/src/router-manifest.ts (1)
11-11: No issues found: getStartManifest API change is fully adoptedVerification confirms only one call site exists (
createStartHandler.ts:216), and it already uses correct object syntax:getStartManifest({ ...opts, routerBasePath: ROUTER_BASEPATH }). No positional argument calls remain in the codebase.e2e/react-start/custom-manifest-base/src/routeTree.gen.ts (1)
1-367: Autogenerated route tree — skipping reviewThis file is generated by TanStack Router; do not hand-edit. Exclude from lint/format and keep changes source-controlled only if required for e2e fixtures. Based on learnings
| "dev": "cross-env NODE_ENV=development tsx express-server.ts", | ||
| "build": "vite build && tsc --noEmit", | ||
| "start": "tsx express-server.ts", | ||
| "test:e2e": "rm -rf port*.txt; playwright test --project=chromium" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use cross-platform command for file deletion.
The rm -rf command in the test:e2e script is Unix-specific and will fail on Windows. Consider using a cross-platform alternative.
Apply this diff to use a cross-platform solution:
- "test:e2e": "rm -rf port*.txt; playwright test --project=chromium"
+ "test:e2e": "rimraf port*.txt && playwright test --project=chromium"And add rimraf to devDependencies:
"postcss": "^8.5.1",
+ "rimraf": "^6.0.1",
"srvx": "^0.8.6",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "test:e2e": "rm -rf port*.txt; playwright test --project=chromium" | |
| "test:e2e": "rimraf port*.txt && playwright test --project=chromium" |
🤖 Prompt for AI Agents
In e2e/react-start/custom-manifest-base/package.json around line 10, the
"test:e2e" script uses Unix-specific "rm -rf" which fails on Windows; replace
the removal with a cross-platform tool by changing the script to use "rimraf"
(e.g. "rimraf port*.txt && playwright test --project=chromium") and add "rimraf"
to devDependencies (install as a dev dependency or add an entry in package.json
devDependencies).
| .catch((err) => { | ||
| console.error(err) | ||
| if (err.status === 404) { | ||
| throw notFound() | ||
| } | ||
| throw err | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect 404 check with redaxios/axios errors
err.status is undefined; use err.response?.status. Current code won’t throw notFound().
- .catch((err) => {
+ .catch((err: any) => {
console.error(err)
- if (err.status === 404) {
+ if (err?.response?.status === 404) {
throw notFound()
}
throw err
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .catch((err) => { | |
| console.error(err) | |
| if (err.status === 404) { | |
| throw notFound() | |
| } | |
| throw err | |
| }) | |
| .catch((err: any) => { | |
| console.error(err) | |
| if (err?.response?.status === 404) { | |
| throw notFound() | |
| } | |
| throw err | |
| }) |
🤖 Prompt for AI Agents
In e2e/react-start/custom-manifest-base/src/utils/posts.tsx around lines 24 to
30, the error handler checks err.status which is undefined for redaxios/axios
errors; change the conditional to inspect the HTTP status on the response (e.g.
use err.response?.status === 404) and throw notFound() when that condition
matches, otherwise rethrow the original error; ensure the access uses optional
chaining to avoid runtime errors.
| if (!serverFnInfo) { | ||
| throw new Error('Server function info not found for ' + id) | ||
| } | ||
| console.info(serverFnInfo["extractedFilename"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove or guard debug logging in production builds.
This console.info statement will execute in production builds without any context message, which can clutter logs and may expose internal file paths.
Consider one of the following options:
Option 1 (recommended): Remove the debug logging entirely
- console.info(serverFnInfo["extractedFilename"])Option 2: Guard with the existing debug flag and add context
- console.info(serverFnInfo["extractedFilename"])
+ if (${debug}) {
+ console.info('[TanStack Server Functions] Loading:', serverFnInfo.extractedFilename)
+ }Note: If keeping the log, use dot notation (serverFnInfo.extractedFilename) for consistency with the rest of the codebase.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.info(serverFnInfo["extractedFilename"]) | |
| // console.info line removed - debug logging should be guarded or removed from production code |
| console.info(serverFnInfo["extractedFilename"]) | |
| // Line containing console.info(serverFnInfo["extractedFilename"]) removed |
🤖 Prompt for AI Agents
In packages/server-functions-plugin/src/index.ts around line 217, there is an
unguarded debug statement `console.info(serverFnInfo["extractedFilename"])` that
will run in production and leaks internal path without context; remove this line
entirely (preferred) or wrap it behind the existing debug flag and add a clear
message and use dot notation (e.g., if (DEBUG) console.debug(`server function
extracted file: ${serverFnInfo.extractedFilename}`)); ensure any retained
logging is at debug level and never runs in production.
| if (!fnModule) { | ||
| console.info('serverFnInfo', serverFnInfo) | ||
| throw new Error('Server function module not resolved for ' + id) | ||
| return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unreachable code.
The return statement on line 223 is unreachable because it follows a throw statement on line 222. This is dead code that should be removed.
Apply this diff to remove the unreachable code:
throw new Error('Server function module not resolved for ' + id)
- return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return | |
| throw new Error('Server function module not resolved for ' + id) | |
| } |
🤖 Prompt for AI Agents
In packages/server-functions-plugin/src/index.ts around line 223, remove the
unreachable "return" statement that follows a "throw" on the previous line;
simply delete the dead return so control flow is correct and there is no
unreachable code left after the throw.
| let script = | ||
| maybeAssetsUrl ? | ||
| `import('${maybeAssetsUrl + "/" + startManifest.clientEntry.replace(routerBasePath ?? '', '')}')` : `import('${startManifest.clientEntry}')` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guard against malformed assetsUrl
Consider validating assetsUrl once (must be absolute HTTP(S)) to fail fast with a clear error, avoiding bad URLs ending up in the client manifest.
Also applies to: 59-72
🤖 Prompt for AI Agents
In packages/start-server-core/src/router-manifest.ts around lines 21-23 (and
similarly 59-72), the code uses maybeAssetsUrl directly to construct import URLs
which can produce malformed client manifests; validate assetsUrl once up-front
to ensure it is a non-empty absolute HTTP or HTTPS URL (scheme http or https),
normalize it (trim whitespace and ensure no double slashes / trailing slash
handling) and throw a clear, early error if validation fails; after validation
use the normalized assetsUrl when building the import string so only well-formed
absolute URLs reach the client manifest.
🛠️ Refactor suggestion | 🟠 Major
Robust import() URL construction and injection‑safe escaping
- Current
replace(routerBasePath ?? '', '')may remove matches mid‑path. - Simple concatenation can yield
//or wrong joins. - Embed string via JSON.stringify to avoid code injection if config is malformed.
- let script =
- maybeAssetsUrl ?
- `import('${maybeAssetsUrl + "/" + startManifest.clientEntry.replace(routerBasePath ?? '', '')}')` : `import('${startManifest.clientEntry}')`
+ const stripPrefix = (p: string, prefix?: string) =>
+ prefix && p.startsWith(prefix) ? p.slice(prefix.length) : p
+ const joinUrl = (base: string, path: string) => {
+ const a = base.replace(/\/+$/, '')
+ const b = path.replace(/^\/+/, '')
+ return `${a}/${b}`
+ }
+ const clientPath = stripPrefix(startManifest.clientEntry, routerBasePath)
+ const clientUrl = maybeAssetsUrl
+ ? joinUrl(maybeAssetsUrl, clientPath)
+ : startManifest.clientEntry
+ let script = `import(${JSON.stringify(clientUrl)})`| preloads: preloads?.map((url) => maybeAssetsUrl + '/' + url.replace(routerBasePath ?? '', '')) || [], | ||
| assets: | ||
| assets?.map((asset) => { | ||
| if (asset.tag === 'link' && asset.attrs?.href) { | ||
| return { | ||
| ...asset, | ||
| attrs: { | ||
| ...asset.attrs, | ||
| href: maybeAssetsUrl + '/' + asset.attrs.href, | ||
| }, | ||
| } | ||
| } | ||
| return asset | ||
| }) || [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Preloads/assets rewriting: handle absolute URLs, strip basepath only at start, and be consistent for links
- Don’t rewrite absolute/data URLs.
- Strip
routerBasePathonly when it’s a leading prefix. - Apply the same stripping for
linkhrefs; current code leaves basepath in place.
- preloads: preloads?.map((url) => maybeAssetsUrl + '/' + url.replace(routerBasePath ?? '', '')) || [],
- assets:
- assets?.map((asset) => {
- if (asset.tag === 'link' && asset.attrs?.href) {
- return {
- ...asset,
- attrs: {
- ...asset.attrs,
- href: maybeAssetsUrl + '/' + asset.attrs.href,
- },
- }
- }
- return asset
- }) || [],
+ preloads:
+ preloads?.map((u) => {
+ const isAbs = /^(?:[a-z][a-z\d+\-.]*:)?\/\//i.test(u) || u.startsWith('data:')
+ if (isAbs) return u
+ const strip = (p: string, prefix?: string) =>
+ prefix && p.startsWith(prefix) ? p.slice(prefix.length) : p
+ const join = (b: string, p: string) =>
+ `${b.replace(/\/+$/, '')}/${p.replace(/^\/+/, '')}`
+ return join(maybeAssetsUrl!, strip(u, routerBasePath))
+ }) || [],
+ assets:
+ assets?.map((asset) => {
+ // Rewrite <link href> when relative
+ if (asset.tag === 'link' && asset.attrs?.href) {
+ const href = asset.attrs.href
+ const isAbs = /^(?:[a-z][a-z\d+\-.]*:)?\/\//i.test(href) || href.startsWith('data:')
+ if (isAbs) return asset
+ const strip = (p: string, prefix?: string) =>
+ prefix && p.startsWith(prefix) ? p.slice(prefix.length) : p
+ const join = (b: string, p: string) =>
+ `${b.replace(/\/+$/, '')}/${p.replace(/^\/+/, '')}`
+ return {
+ ...asset,
+ attrs: {
+ ...asset.attrs,
+ href: join(maybeAssetsUrl!, strip(href, routerBasePath)),
+ },
+ }
+ }
+ return asset
+ }) || [],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| preloads: preloads?.map((url) => maybeAssetsUrl + '/' + url.replace(routerBasePath ?? '', '')) || [], | |
| assets: | |
| assets?.map((asset) => { | |
| if (asset.tag === 'link' && asset.attrs?.href) { | |
| return { | |
| ...asset, | |
| attrs: { | |
| ...asset.attrs, | |
| href: maybeAssetsUrl + '/' + asset.attrs.href, | |
| }, | |
| } | |
| } | |
| return asset | |
| }) || [], | |
| preloads: | |
| preloads?.map((u) => { | |
| const isAbs = /^(?:[a-z][a-z\d+\-.]*:)?\/\//i.test(u) || u.startsWith('data:') | |
| if (isAbs) return u | |
| const strip = (p: string, prefix?: string) => | |
| prefix && p.startsWith(prefix) ? p.slice(prefix.length) : p | |
| const join = (b: string, p: string) => | |
| `${b.replace(/\/+$/, '')}/${p.replace(/^\/+/, '')}` | |
| return join(maybeAssetsUrl!, strip(u, routerBasePath)) | |
| }) || [], | |
| assets: | |
| assets?.map((asset) => { | |
| // Rewrite <link href> when relative | |
| if (asset.tag === 'link' && asset.attrs?.href) { | |
| const href = asset.attrs.href | |
| const isAbs = /^(?:[a-z][a-z\d+\-.]*:)?\/\//i.test(href) || href.startsWith('data:') | |
| if (isAbs) return asset | |
| const strip = (p: string, prefix?: string) => | |
| prefix && p.startsWith(prefix) ? p.slice(prefix.length) : p | |
| const join = (b: string, p: string) => | |
| `${b.replace(/\/+$/, '')}/${p.replace(/^\/+/, '')}` | |
| return { | |
| ...asset, | |
| attrs: { | |
| ...asset.attrs, | |
| href: join(maybeAssetsUrl!, strip(href, routerBasePath)), | |
| }, | |
| } | |
| } | |
| return asset | |
| }) || [], |
🤖 Prompt for AI Agents
In packages/start-server-core/src/router-manifest.ts around lines 59 to 72,
update the preloads and assets href rewriting so you (1) skip rewriting for
absolute URLs or data URLs (detect schemes like "data:", protocol-relative "//"
or any "scheme:"); (2) only strip routerBasePath when it is a leading prefix
(use startsWith check) rather than replacing occurrences anywhere; and (3) apply
the same leading-prefix stripping logic to link hrefs as you do for preload
entries so both paths are handled consistently. Ensure you treat null/undefined
routerBasePath safely and return the original URL when in-scope checks indicate
no rewriting is needed.
Summary by CodeRabbit
New Features
Chores