fix: basepath #5345
fix: basepath #5345
Conversation
WalkthroughRemoves the exported rewriteBasepath and migrates basepath handling into runtime/router options and plugin config. Updates Vite/Nitro/start plugins, client/server RPC and manifest flows, tests, and example Vite base setting to use the new basepath/environment variables and normalized config plumbing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor DevBrowser as Browser
participant Vite as Vite
participant StartPlugin as Start Plugin
participant ClientApp as Client App
participant Router as RouterCore
Note over Vite,StartPlugin: build/dev config resolution
Vite->>StartPlugin: configResolved(config)
StartPlugin->>StartPlugin: derive viteAppBase & router.basepath
StartPlugin-->>ClientApp: define env TSS_ROUTER_BASEPATH, TSS_SERVER_FN_BASE
Note over ClientApp,Router: hydration/update
ClientApp->>Router: router.update({ basepath: TSS_ROUTER_BASEPATH, ... })
Router->>Router: compute basepath rewrite, compose with rewrites
Router-->>ClientApp: normalized location
ClientApp-->>DevBrowser: render with basepath-aware routes
sequenceDiagram
autonumber
participant Client as Browser
participant Server as Start Server
participant Router as RouterCore
participant Manifest as Manifest Loader
participant SrvFn as Server Functions Handler
Client->>Server: HTTP request
alt Server Function URL
Server->>SrvFn: handleServerAction(req)
SrvFn->>SrvFn: lazy-init regex from TSS_SERVER_FN_BASE
SrvFn-->>Server: invoke server function
Server-->>Client: function response
else App route
Server->>Router: router.update({ basepath: TSS_ROUTER_BASEPATH, ... })
Server->>Manifest: getStartManifest()
Manifest-->>Server: startManifest
Server-->>Client: HTML/Response (returned directly)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
packages/{*-start,start-*}/**📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧬 Code graph analysis (1)packages/start-plugin-core/src/plugin.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (6)
Comment |
|
View your CI Pipeline Execution ↗ for commit 7b1b684
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/start-plugin-core/src/prerender.ts (1)
141-185: Restore leading slash before callingwithBase.
startConfig.router.basepathis normalized elsewhere to strip leading/trailing slashes, so values like'foo'reach here. Passing that directly towithBaseproduces'foo/...', which makeslocalFetchbuild bad URLs (http://localhostfoo/...) and causes prerender fetches/writes to miss the intended base path.Add back the leading slash before composing URLs, e.g.:
- const routerBasePath = startConfig.router.basepath || '/' + const routerBasePath = joinURL('/', startConfig.router.basepath ?? '')
🧹 Nitpick comments (1)
packages/start-server-core/src/createStartHandler.ts (1)
174-174: Direct env var access without fallback.Unlike line 57 which provides a fallback for
TSS_ROUTER_BASEPATH, this line directly accessesprocess.env.TSS_SERVER_FN_BASE. If the env var is not set,href.startsWith(undefined)will be called, which convertsundefinedto"undefined"and performs a string comparison that will always be false.While the current behavior is safe (won't crash), it's less clear than an explicit check. Consider adding a constant with validation:
const SERVER_FN_BASE = process.env.TSS_SERVER_FN_BASE if (!SERVER_FN_BASE) { throw new Error('TSS_SERVER_FN_BASE environment variable is required') } // Later at line 174: if (href.startsWith(SERVER_FN_BASE)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
examples/react/start-basic/vite.config.ts(1 hunks)packages/react-router/src/index.tsx(1 hunks)packages/react-router/tests/router.test.tsx(11 hunks)packages/router-core/src/index.ts(0 hunks)packages/router-core/src/router.ts(3 hunks)packages/solid-router/src/index.tsx(1 hunks)packages/start-client-core/src/client-rpc/createClientRpc.ts(1 hunks)packages/start-client-core/src/client/hydrateStart.ts(1 hunks)packages/start-plugin-core/src/global.d.ts(0 hunks)packages/start-plugin-core/src/plugin.ts(6 hunks)packages/start-plugin-core/src/prerender.ts(3 hunks)packages/start-plugin-core/src/schema.ts(1 hunks)packages/start-plugin-core/src/start-manifest-plugin/plugin.ts(6 hunks)packages/start-server-core/src/createServerRpc.ts(1 hunks)packages/start-server-core/src/createStartHandler.ts(4 hunks)packages/start-server-core/src/global.d.ts(1 hunks)packages/start-server-core/src/request-response.ts(1 hunks)packages/start-server-core/src/router-manifest.ts(1 hunks)packages/start-server-core/src/server-functions-handler.ts(2 hunks)
💤 Files with no reviewable changes (2)
- packages/router-core/src/index.ts
- packages/start-plugin-core/src/global.d.ts
🧰 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:
packages/start-server-core/src/router-manifest.tsexamples/react/start-basic/vite.config.tspackages/react-router/src/index.tsxpackages/start-server-core/src/global.d.tspackages/start-server-core/src/createServerRpc.tspackages/start-client-core/src/client/hydrateStart.tspackages/start-plugin-core/src/plugin.tspackages/start-server-core/src/createStartHandler.tspackages/start-plugin-core/src/start-manifest-plugin/plugin.tspackages/start-server-core/src/request-response.tspackages/start-plugin-core/src/prerender.tspackages/start-plugin-core/src/schema.tspackages/react-router/tests/router.test.tsxpackages/router-core/src/router.tspackages/solid-router/src/index.tsxpackages/start-server-core/src/server-functions-handler.tspackages/start-client-core/src/client-rpc/createClientRpc.ts
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/router-manifest.tspackages/start-server-core/src/global.d.tspackages/start-server-core/src/createServerRpc.tspackages/start-client-core/src/client/hydrateStart.tspackages/start-plugin-core/src/plugin.tspackages/start-server-core/src/createStartHandler.tspackages/start-plugin-core/src/start-manifest-plugin/plugin.tspackages/start-server-core/src/request-response.tspackages/start-plugin-core/src/prerender.tspackages/start-plugin-core/src/schema.tspackages/start-server-core/src/server-functions-handler.tspackages/start-client-core/src/client-rpc/createClientRpc.ts
examples/{react,solid}/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep example applications under examples/react/ and examples/solid/
Files:
examples/react/start-basic/vite.config.ts
packages/{react-router,solid-router}/**
📄 CodeRabbit inference engine (AGENTS.md)
Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/
Files:
packages/react-router/src/index.tsxpackages/react-router/tests/router.test.tsxpackages/solid-router/src/index.tsx
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/src/router.ts
🧬 Code graph analysis (4)
packages/start-plugin-core/src/plugin.ts (1)
packages/router-core/src/index.ts (2)
trimPathRight(98-98)joinPaths(95-95)
packages/start-server-core/src/createStartHandler.ts (1)
packages/start-server-core/src/router-manifest.ts (1)
getStartManifest(11-59)
packages/start-plugin-core/src/start-manifest-plugin/plugin.ts (1)
packages/start-plugin-core/src/plugin.ts (1)
GetConfigFn(47-50)
packages/router-core/src/router.ts (1)
packages/router-core/src/rewrite.ts (1)
rewriteBasepath(21-55)
🪛 ast-grep (0.39.5)
packages/start-server-core/src/server-functions-handler.ts
[warning] 27-27: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(${process.env.TSS_SERVER_FN_BASE}([^/?#]+))
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (8)
packages/start-server-core/src/request-response.ts (1)
48-59: Remove commented-outh3_toResponsecall
TheRequestHandleralready returns aResponse(orPromise<Response>), and there are no remainingtoResponsereferences—delete the// h3_toResponse(response, h3Event)on line 57.packages/start-plugin-core/src/plugin.ts (1)
103-106: Fix invalidString.replacecall.
String.prototype.replaceaccepts only two arguments, but this call passes three, triggering a type error and breaking the build. Drop the superfluous empty string:- startConfig.router.basepath = startConfig.router.basepath.replace( - /^\/|\/$/g, - '', - '', - ) + startConfig.router.basepath = startConfig.router.basepath.replace( + /^\/|\/$/g, + '', + )Likely an incorrect or invalid review comment.
packages/start-server-core/src/server-functions-handler.ts (2)
27-29: ReDoS warning is a false positive for this pattern.The static analysis tool flags regexp construction from variable input as a potential ReDoS risk. However, the pattern
${process.env.TSS_SERVER_FN_BASE}([^/?#]+)is safe because:
- The interpolated
TSS_SERVER_FN_BASEis a build-time/config-time constant, not user input- The capture group
[^/?#]+uses a negated character class that matches efficiently without backtracking- The pattern is bounded by URL structure (no nested quantifiers or alternations)
Based on static analysis hints.
13-13: Lazy initialization pattern is efficient.The module-level regex with lazy initialization on first call is a good performance optimization, avoiding regex compilation on module load.
Also applies to: 27-29
packages/start-server-core/src/createStartHandler.ts (2)
57-57: Fallback to '/' is good defensive programming.Despite
global.d.tsdeclaringTSS_ROUTER_BASEPATHas required, providing a fallback to'/'is prudent defensive programming that prevents runtime errors. However, this creates an inconsistency between the type declaration and actual usage.Consider updating
global.d.tsto makeTSS_ROUTER_BASEPATHoptional to match this defensive usage, or document that the fallback is intentional for backwards compatibility.
211-211: LGTM: Aligns with updated getStartManifest signature.The call to
getStartManifest()without arguments correctly matches the updated signature that removed thebasePathparameter.packages/start-server-core/src/router-manifest.ts (1)
11-11: Breaking change: remove basePath parameter
All internal calls now usegetStartManifest()without arguments—verified. Update any external callers accordingly.packages/start-client-core/src/client-rpc/createClientRpc.ts (1)
5-5: TSS_SERVER_FN_BASE is already normalized with a single trailing slash
ThejoinPaths([... '/', basepath, serverFns.base, '/' ])+cleanPathcollapse duplicate slashes and guarantee exactly one leading and trailing slash.
| TSS_ROUTER_BASEPATH: string | ||
| TSS_SERVER_FN_BASE: string |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Required env vars need runtime validation.
Changing TSS_ROUTER_BASEPATH and TSS_SERVER_FN_BASE from optional to required in the type system doesn't guarantee they're set at runtime. If these aren't provided, process.env.TSS_ROUTER_BASEPATH will be undefined despite the type declaring it as string, leading to potential runtime errors.
Consider adding runtime validation where these are first accessed (e.g., in createStartHandler.ts line 57 and createClientRpc.ts line 5):
const ROUTER_BASEPATH = process.env.TSS_ROUTER_BASEPATH
if (!ROUTER_BASEPATH) {
throw new Error('TSS_ROUTER_BASEPATH environment variable is required')
}Similarly for TSS_SERVER_FN_BASE:
const SERVER_FN_BASE = process.env.TSS_SERVER_FN_BASE
if (!SERVER_FN_BASE) {
throw new Error('TSS_SERVER_FN_BASE environment variable is required')
}🤖 Prompt for AI Agents
In packages/start-server-core/src/global.d.ts around lines 4 to 5, the env vars
TSS_ROUTER_BASEPATH and TSS_SERVER_FN_BASE were made required in types but lack
runtime checks; add runtime validation at the points they’re first accessed
(e.g., in createStartHandler.ts at ~line 57 and createClientRpc.ts at ~line 5):
read each value from process.env, verify it is a non-empty string, and throw a
clear Error if missing so the runtime matches the type expectations and fails
fast with an actionable message.
schiller-manuel
force-pushed
the
fix-basepath
branch
from
6f50f8e to
f8e36da
Compare
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/nitro-v2-vite-plugin
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-ssr-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-server
@tanstack/router-cli
@tanstack/router-core
@tanstack/router-devtools
@tanstack/router-devtools-core
@tanstack/router-generator
@tanstack/router-plugin
@tanstack/router-ssr-query-core
@tanstack/router-utils
@tanstack/router-vite-plugin
@tanstack/server-functions-plugin
@tanstack/solid-router
@tanstack/solid-router-devtools
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-static-server-functions
@tanstack/start-storage-context
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
schiller-manuel
force-pushed
the
fix-basepath
branch
from
fa41d59 to
c79ff93
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/start-plugin-core/src/prerender.ts (1)
141-141: HoistrouterBasePathoutside the loop for efficiency.The
routerBasePathis recomputed for every page insideaddCrawlPageTask, butstartConfig.router.basepathremains constant throughout the prerender operation. Declare it once in theprerenderPagesfunction scope to avoid redundant computations.Apply this diff:
async function prerenderPages({ outputDir }: { outputDir: string }) { const seen = new Set<string>() const retriesByPath = new Map<string, number>() const concurrency = startConfig.prerender?.concurrency ?? os.cpus().length logger.info(`Concurrency: ${concurrency}`) const queue = new Queue({ concurrency }) + const routerBasePath = joinURL('/', startConfig.router.basepath ?? '') startConfig.pages.forEach((page) => addCrawlPageTask(page)) await queue.start() return Array.from(seen) function addCrawlPageTask(page: Page) { // Was the page already seen? if (seen.has(page.path)) return // Add the page to the seen set seen.add(page.path) if (page.fromCrawl) { startConfig.pages.push(page) } // If not enabled, skip if (!(page.prerender?.enabled ?? true)) return // If there is a filter link, check if the page should be prerendered if (startConfig.prerender?.filter && !startConfig.prerender.filter(page)) return // Resolve the merged default and page-specific prerender options const prerenderOptions = { ...startConfig.prerender, ...page.prerender, } - const routerBasePath = joinURL('/', startConfig.router.basepath ?? '') - // Add the taskpackages/nitro-v2-vite-plugin/src/index.ts (1)
10-17: Consider extractingisFullUrlto a shared utility.The
isFullUrlhelper is duplicated across multiple packages (also present instart-plugin-core). If these packages share a common utilities package, consider extracting this function to reduce duplication and ensure consistent behavior.If a shared utilities package exists (e.g.,
@tanstack/start-utils), extract the helper:// In shared utils package export function isFullUrl(str: string): boolean { try { new URL(str) return true } catch { return false } }Then import it in both files:
+import { isFullUrl } from '@tanstack/start-utils' + -function isFullUrl(str: string): boolean { - try { - new URL(str) - return true - } catch { - return false - } -}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/nitro-v2-vite-plugin/src/index.ts(3 hunks)packages/react-router/tests/router.test.tsx(11 hunks)packages/start-plugin-core/src/plugin.ts(6 hunks)packages/start-plugin-core/src/prerender.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/start-plugin-core/src/plugin.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/start-plugin-core/src/prerender.tspackages/react-router/tests/router.test.tsxpackages/nitro-v2-vite-plugin/src/index.ts
packages/{*-start,start-*}/**
📄 CodeRabbit inference engine (AGENTS.md)
Name and place Start framework packages under packages/-start/ or packages/start-/
Files:
packages/start-plugin-core/src/prerender.ts
packages/{react-router,solid-router}/**
📄 CodeRabbit inference engine (AGENTS.md)
Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/
Files:
packages/react-router/tests/router.test.tsx
🧬 Code graph analysis (1)
packages/nitro-v2-vite-plugin/src/index.ts (1)
packages/start-plugin-core/src/plugin.ts (1)
config(104-306)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (4)
packages/react-router/tests/router.test.tsx (1)
2604-3005: LGTM! Comprehensive test coverage for basepath functionality.The new test suite thoroughly covers the
basepathoption across multiple scenarios:
- Basic basepath rewriting (stripping on input)
- Various basepath formats (with/without leading/trailing slashes)
- Edge cases (empty basepath, trailing slash mismatches)
- Composition with additional rewrite logic
- Preservation of search params and hash
- Output rewriting (adding basepath back on navigation)
- Nested rewrites with multiple layers
The parameterized tests using
it.eacheffectively validate that basepath works consistently regardless of slash conventions, which is a common source of bugs.packages/nitro-v2-vite-plugin/src/index.ts (3)
10-17: LGTM!The
isFullUrlhelper correctly identifies absolute URLs using the URL constructor. The try-catch pattern handles all edge cases appropriately (empty strings, relative paths, protocol-relative URLs).
20-20: Use a definite assignment assertion for resolvedConfig
TypeScript’s strictPropertyInitialization may flag the uninitialized declaration—change tolet resolvedConfig!: ResolvedConfigor disable
strictPropertyInitializationin your tsconfig.
97-108: Review nitroConfig spread orderingThe spread of
...nitroConfigcurrently followsbaseURLandpublicAssets, so user settings will override those computed defaults. If defaults should take precedence, move the spread above them; otherwise document or warn when overrides occur.packages/nitro-v2-vite-plugin/src/index.ts (around line 108)
Suggested change:
const config: NitroConfig = { + ...nitroConfig, baseURL, publicAssets: [ { dir: client.config.build.outDir, maxAge: 31536000 }, ], - ...nitroConfig, renderer: virtualEntry, }
schiller-manuel
force-pushed
the
fix-basepath
branch
from
c79ff93 to
85e2afd
Compare
apply the same to nitro-vite2-plugin
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/nitro-v2-vite-plugin/src/index.ts (1)
10-17: Consider extractingisFullUrlto a shared utility module.This function is duplicated in
packages/start-plugin-core/src/plugin.ts(line 103). To adhere to DRY principles, consider extracting it to a shared utility module that both packages can import.packages/start-plugin-core/src/plugin.ts (1)
52-59: ExtractisFullUrlinto a shared utility
Identical implementations exist inpackages/start-plugin-core/src/plugin.tsandpackages/nitro-v2-vite-plugin/src/index.ts; centralize this helper to adhere to DRY principles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/nitro-v2-vite-plugin/src/index.ts(3 hunks)packages/start-plugin-core/src/plugin.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/start-plugin-core/src/plugin.tspackages/nitro-v2-vite-plugin/src/index.ts
packages/{*-start,start-*}/**
📄 CodeRabbit inference engine (AGENTS.md)
Name and place Start framework packages under packages/-start/ or packages/start-/
Files:
packages/start-plugin-core/src/plugin.ts
🧬 Code graph analysis (2)
packages/start-plugin-core/src/plugin.ts (1)
packages/start-plugin-core/src/schema.ts (1)
TanStackStartOutputConfig(202-202)
packages/nitro-v2-vite-plugin/src/index.ts (1)
packages/start-plugin-core/src/plugin.ts (1)
config(104-313)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (9)
packages/nitro-v2-vite-plugin/src/index.ts (2)
20-20: Verify hook execution order or guardresolvedConfigusage.No references were found in the repo confirming that
configResolvedalways runs before thebuildAppcallback. Please manually verify Vite’s lifecycle for your version or add a runtime check before usingresolvedConfigto prevent undefined access.
97-109: Review config override and publicAssets baseURL
- user-provided nitroConfig.baseURL overrides the computed baseURL due to spread order; swap them if the computed value should take precedence.
- publicAssets.baseURL is hardcoded to
'/'and may conflict with a non-root base path; either align it to the computed baseURL or document why it must remain'/'.packages/start-plugin-core/src/plugin.ts (7)
1-1: LGTM!The import of
joinPathsis necessary for the new path normalization logic introduced in this PR.
44-44: LGTM!The
viteAppBaseproperty is properly typed and integrated into the configuration flow.
70-70: LGTM!The empty string default is appropriate and will be overridden during configuration resolution.
126-131: LGTM!The computation correctly derives the server function base path from the router basepath and serverFns base, ensuring consistent path handling throughout the application.
338-341: LGTM!The refactoring to pass
getConfiginstead of individual parameters improves maintainability and consistency with other plugins likedevServerPlugin.
280-282: Migration verified: no remainingTSS_APP_BASEreferences
Ripgrep search returned no hits in.ts/.tsx.
117-124: Default/basepath becoming''is treated as root and works correctly.
| resolvedStartConfig.viteAppBase = viteConfig.base ?? '/' | ||
| if (!isFullUrl(resolvedStartConfig.viteAppBase)) { | ||
| resolvedStartConfig.viteAppBase = joinPaths([ | ||
| '/', | ||
| viteConfig.base, | ||
| '/', | ||
| ]) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify handling of empty string base path.
The logic correctly distinguishes between full URLs and relative paths. However, if viteConfig.base is explicitly set to an empty string (''), it will be normalized to '/'. Ensure this behavior aligns with Vite's base path handling conventions.
Run the following script to check Vite's default base handling:
🌐 Web query:
What is the default value and behavior of Vite's config.base option when set to an empty string?
💡 Result:
Default is "/". Setting config.base to an empty string (or "./") tells Vite to use a relative (embedded) public base path so built asset URLs are generated relative to the current HTML file (suitable for embedded/static deployments). import.meta.env.BASE_URL will reflect that relative value. [1][2]
Sources:
[1] Vite config (base option).
[2] Vite — Public Base Path / Building for Production.
🏁 Script executed:
#!/bin/bash
rg -n "joinPaths" -C3 --type=tsLength of output: 9666
🏁 Script executed:
#!/bin/bash
sed -n '1,100p' packages/router-core/src/path.tsLength of output: 2691
Handle explicit empty-string base path. Vite treats base: "" as a relative ./ public path, but here viteConfig.base === "" is normalized to /. Add a guard before the URL check to set resolvedStartConfig.viteAppBase = "./" when viteConfig.base === "".
🤖 Prompt for AI Agents
In packages/start-plugin-core/src/plugin.ts around lines 105 to 112, add a guard
for the explicit empty-string base: if viteConfig.base === "" set
resolvedStartConfig.viteAppBase = "./" before the URL check; otherwise continue
with the existing logic that sets viteAppBase to viteConfig.base ?? '/' and
falls back to joinPaths when not a full URL. This ensures Vite's empty-string
base is preserved as a relative "./" path instead of being normalized to "/".
02c743b to
9f71f75
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/start-plugin-core/src/plugin.ts (1)
105-124: Preserve explicit empty-string Vite base.Vite treats
config.base === ''as the special embedded-relative./base, but the normalization here collapses it back to'/', so relative deployments andimport.meta.env.BASE_URL === './'break. Please keep the empty-string case intact (setviteAppBaseto'./') and translate that to an empty router basepath instead of'.'.- resolvedStartConfig.viteAppBase = viteConfig.base ?? '/' - if (!isFullUrl(resolvedStartConfig.viteAppBase)) { - resolvedStartConfig.viteAppBase = joinPaths([ - '/', - viteConfig.base, - '/', - ]) - } + const rawViteBase = viteConfig.base ?? '/' + if (viteConfig.base === '') { + resolvedStartConfig.viteAppBase = './' + } else if (!isFullUrl(rawViteBase)) { + resolvedStartConfig.viteAppBase = joinPaths([ + '/', + rawViteBase, + '/', + ]) + } else { + resolvedStartConfig.viteAppBase = rawViteBase + } @@ - if (!isFullUrl(resolvedStartConfig.viteAppBase)) { - startConfig.router.basepath = - resolvedStartConfig.viteAppBase.replace(/^\/|\/$/g, '') + if (!isFullUrl(resolvedStartConfig.viteAppBase)) { + const normalizedBase = resolvedStartConfig.viteAppBase.replace( + /^\/|\/$/g, + '', + ) + startConfig.router.basepath = + normalizedBase === '.' ? '' : normalizedBase } else { startConfig.router.basepath = '/' }
🧹 Nitpick comments (1)
packages/nitro-v2-vite-plugin/src/index.ts (1)
20-20: Consider initializingresolvedConfigor using definite assignment assertion.In TypeScript strict mode,
resolvedConfigis declared but not initialized. While Vite's lifecycle guaranteesconfigResolvedruns beforebuildApp, the type system doesn't know this.Apply one of these approaches:
Option 1: Definite assignment assertion (if you're certain about the lifecycle)
- let resolvedConfig: ResolvedConfig + let resolvedConfig!: ResolvedConfigOption 2: Defensive check in buildApp
async buildApp(builder) { + if (!resolvedConfig) { + throw new Error('configResolved hook must run before buildApp') + } const client = builder.environments.clientAlso applies to: 55-57
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/nitro-v2-vite-plugin/src/index.ts(3 hunks)packages/start-plugin-core/src/plugin.ts(6 hunks)packages/start-plugin-core/src/prerender.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/start-plugin-core/src/prerender.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/nitro-v2-vite-plugin/src/index.tspackages/start-plugin-core/src/plugin.ts
packages/{*-start,start-*}/**
📄 CodeRabbit inference engine (AGENTS.md)
Name and place Start framework packages under packages/-start/ or packages/start-/
Files:
packages/start-plugin-core/src/plugin.ts
🧬 Code graph analysis (1)
packages/start-plugin-core/src/plugin.ts (1)
packages/start-plugin-core/src/schema.ts (1)
TanStackStartOutputConfig(202-202)
🔇 Additional comments (4)
packages/nitro-v2-vite-plugin/src/index.ts (4)
4-4: LGTM!The addition of
ResolvedConfigto the imports is necessary for the newconfigResolvedhook.
10-17: LGTM!The
isFullUrlhelper correctly uses the URL constructor to detect absolute URLs. The try-catch pattern appropriately handles theTypeErrorthrown for relative URLs.
97-99: LGTM!The logic correctly filters out absolute URLs (e.g., from CDN configurations) and only uses relative base paths as the Nitro
baseURL.
100-121: Verify thatpublicAssetsoverride behavior is intentional.The current structure allows
nitroConfigto completely override thepublicAssetsarray (which includes the client build output). If a user provides their ownpublicAssets, the client assets might not be served correctly.Consider whether you should merge the arrays instead:
const config: NitroConfig = { baseURL, - publicAssets: [ - { - dir: client.config.build.outDir, - maxAge: 31536000, // 1 year - baseURL: '/', - }, - ], ...nitroConfig, + publicAssets: [ + { + dir: client.config.build.outDir, + maxAge: 31536000, // 1 year + baseURL: '/', + }, + ...(nitroConfig?.publicAssets ?? []), + ], renderer: virtualEntry,This ensures the client assets are always served while allowing users to add additional public assets.
schiller-manuel
force-pushed
the
fix-basepath
branch
from
841d6cf to
7b1b684
Compare
1 participant
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores