Uh oh!
There was an error while loading. Please reload this page.
feat: support split base and basepath in TanStack Start dev server - #6792
Merged
Conversation
Contributor
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a new e2e fixture (split-base-and-basepath) with router routes, tests, and Vite/Playwright configs; introduces dev-time URL-rewrite middleware in start-plugin-core to handle mismatched Vite base vs router basepath; removes a Changes
Sequence DiagramsequenceDiagram
participant Client as Browser/Client
participant DevRewrite as Dev Base Rewrite Middleware
participant ViteBase as Vite Base Middleware
participant SSR as SSR Handler
Note over Client,SSR: Dev-mode request when router basepath != Vite base
Client->>DevRewrite: GET /about
DevRewrite->>DevRewrite: detects Vite base '/_ui/' and rewrites URL
DevRewrite->>ViteBase: GET /_ui/about
ViteBase->>ViteBase: strips '/_ui/' prefix
ViteBase->>SSR: GET /about
SSR->>SSR: render HTML with asset URLs prefixed '/_ui/'
SSR->>ViteBase: return HTML
ViteBase->>DevRewrite: pass-through
DevRewrite->>Client: HTML response (assets use /_ui/*)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Contributor
View your CI Pipeline Execution ↗ for commit bec3134
☁️ Nx Cloud last updated this comment at |
Contributor
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
e2e/react-start/split-base-and-basepath/playwright.config.ts (1)
21-27: Remove duplicatedPORTinjection in prod command.
webServer.env.PORTalready provides the variable; keeping it only there reduces command-string coupling.♻️ Suggested patch
webServer: { - command: isDev ? `pnpm dev:e2e` : `pnpm build && PORT=${PORT} pnpm start`,+ command: isDev ? `pnpm dev:e2e` : `pnpm build && pnpm start`, url: baseURL, reuseExistingServer: !process.env.CI, stdout: 'pipe',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/react-start/split-base-and-basepath/playwright.config.ts` around lines 21 - 27, The prod command in the Playwright webServer config duplicates PORT by embedding PORT=${PORT} in the "command" string while "env" already sets PORT; update the "command" property used when isDev is false to remove the embedded PORT=${PORT} so it relies on the webServer.env.PORT value (locate the "command" property and the webServer "env" object in playwright.config.ts and remove the PORT injection from the non-dev command).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/react-start/split-base-and-basepath/package.json`:
- Around line 18-26: The package.json shows a runtime dependency "express" at v4
while devDependencies use "@types/express" v5 causing type mismatches; update
the dependency pair so their major versions match—either change "@types/express"
to a v4 release (e.g., ^4.x) to match "express": "^4.21.2", or upgrade "express"
to v5 and set "@types/express" to the corresponding v5; apply the same alignment
to any other e2e fixture package.json files that list "express" and
"@types/express".
In `@e2e/react-start/split-base-and-basepath/tests/app.spec.ts`:
- Around line 12-14: The test currently parses HTML from the fetch without first
asserting the HTTP response succeeded; add an explicit success assertion
immediately after `const response = await request.get('/')` (for example
`ok(response.ok)` or `assert.strictEqual(response.status, 200)`) so request
failures surface as a clear test/setup error before `const html = await
response.text()` and downstream regex/assert noise; update the test near the
`request.get('/')` call to perform this check.
In `@packages/start-plugin-core/src/plugin.ts`:
- Around line 413-415: The rewrite logic that prefixes req.url with base can
double-prefix when req.url already equals the base root with or without a
trailing slash or query (e.g., "/_ui" or "/_ui?x=1"); update the check to
normalize base (e.g., let normalizedBase = base.replace(/\/$/, '')) and only
prefix when req.url does not already start with normalizedBase nor
normalizedBase + '/' nor equal normalizedBase (and handle query strings
implicitly), i.e., replace the current if (req.url && !req.url.startsWith(base))
{ ... } with a comparison against the normalizedBase variants using the existing
req.url and base symbols so URLs at the base root are not rewritten.
---
Nitpick comments:
In `@e2e/react-start/split-base-and-basepath/playwright.config.ts`:
- Around line 21-27: The prod command in the Playwright webServer config
duplicates PORT by embedding PORT=${PORT} in the "command" string while "env"
already sets PORT; update the "command" property used when isDev is false to
remove the embedded PORT=${PORT} so it relies on the webServer.env.PORT value
(locate the "command" property and the webServer "env" object in
playwright.config.ts and remove the PORT injection from the non-dev command).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
e2e/react-start/import-protection/vite.config.tse2e/react-start/split-base-and-basepath/.gitignoree2e/react-start/split-base-and-basepath/.prettierignoree2e/react-start/split-base-and-basepath/package.jsone2e/react-start/split-base-and-basepath/playwright.config.tse2e/react-start/split-base-and-basepath/prod-server.jse2e/react-start/split-base-and-basepath/src/routeTree.gen.tse2e/react-start/split-base-and-basepath/src/router.tsxe2e/react-start/split-base-and-basepath/src/routes/__root.tsxe2e/react-start/split-base-and-basepath/src/routes/about.tsxe2e/react-start/split-base-and-basepath/src/routes/index.tsxe2e/react-start/split-base-and-basepath/src/styles/app.csse2e/react-start/split-base-and-basepath/tests/app.spec.tse2e/react-start/split-base-and-basepath/tsconfig.jsone2e/react-start/split-base-and-basepath/vite.config.tspackages/start-plugin-core/src/plugin.ts
💤 Files with no reviewable changes (1)
- e2e/react-start/import-protection/vite.config.ts
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores