fix: dev style with non runnable dev environments and basepath support #6348
Conversation
📝 WalkthroughWalkthroughAdds a shared buildDevStylesUrl utility and uses it in framework HeadContent; consolidates dev-styles CSS middleware into a single always-installed handler; adds Nitro/Cloudflare/basepath e2e variants, scripts, and configs; makes Vite/Playwright env-driven; broadens workspace specifiers to Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Playwright as TestRunner
participant Vite as DevServer
participant Plugin as StartPluginCore
participant Manifest as TSS_ROUTES_MANIFEST
participant Collector as collectDevStyles
Browser->>Playwright: request page (may include basePath)
Playwright->>Vite: request /@tanstack-start/styles.css?routes=...
Vite->>Plugin: handle dev-styles request
Plugin->>Manifest: validate & resolve route IDs -> file paths
Manifest-->>Plugin: file paths (or missing)
Plugin->>Collector: collectDevStyles(file paths)
Collector-->>Plugin: aggregated CSS
Plugin-->>Vite: respond with CSS (or CSS comment on error)
Vite-->>Playwright: deliver CSS
Playwright-->>Browser: apply CSS / continue tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
Comment |
|
View your CI Pipeline Execution ↗ for commit 53f8a89
☁️ 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/router-core/src/index.ts (1)
266-279: Public API addition: consider documentingbuildDevStylesUrl(or adding a small unit test) to lock behavior.packages/start-plugin-core/src/dev-server-plugin/plugin.ts (1)
47-51: Fix dev-styles middleware path matching for basepath (currently misses/<basepath>/@tanstack-start/styles.css).
GivenbuildDevStylesUrl(router.basepath, ...), requests can be basepath-prefixed; the currentstartsWith('/@tanstack-start/styles.css')check won’t catch them, breaking dev styles under basepath.Proposed fix
- viteDevServer.middlewares.use(async (req, res, next) => { - const url = req.url ?? '' - if (!url.startsWith('/@tanstack-start/styles.css')) { - return next() - } + viteDevServer.middlewares.use(async (req, res, next) => { + const rawUrl = req.originalUrl ?? req.url ?? '' + // Fast-path: avoid URL parsing when clearly unrelated + if (!rawUrl.includes('/@tanstack-start/styles.css')) { + return next() + } try { - // Parse route IDs from query param - const urlObj = new URL(url, 'http://localhost') + // Parse route IDs from query param + const urlObj = new URL(rawUrl, 'http://localhost') + if (!urlObj.pathname.endsWith('/@tanstack-start/styles.css')) { + return next() + } const routesParam = urlObj.searchParams.get('routes') - const routeIds = routesParam ? routesParam.split(',') : [] + const routeIds = routesParam + ? routesParam.split(',').filter(Boolean) + : []Also applies to: 53-105
🤖 Fix all issues with AI agents
In @e2e/react-start/css-modules/playwright.config.ts:
- Around line 11-17: The test harness builds baseURL and devCommand from
process.env.VITE_CONFIG (via the local viteConfig variable) but does not
propagate that variable into the Playwright webServer.env, making the dev server
unaware of the same config; update the webServer.env block in
playwright.config.ts (the webServer configuration object) to include VITE_CONFIG
with the same value you compute for viteConfig (e.g. VITE_CONFIG: viteConfig ||
''), so the spawned dev server receives the same VITE_CONFIG used to construct
baseURL and devCommand.
In @e2e/react-start/css-modules/tests/css.spec.ts:
- Around line 169-171: The waitForURL call in the test uses a regex that
requires a trailing slash to match home, so it fails when a basepath has no
trailing slash; update the argument passed to page.waitForURL in css.spec.ts
(the page.waitForURL(...) call) to use a regex that allows an optional trailing
slash or a query or end-of-string (i.e., accept either immediate end, a trailing
'/' or a '?' after the path) so both "/my-app" and "/my-app/" are matched.
In @packages/router-core/src/utils.ts:
- Around line 603-620: The basepath normalization in buildDevStylesUrl can
produce double slashes for empty or trailing-slash basepaths; fix it by trimming
all leading and trailing slashes from the input (e.g., using a regex like
/^\/+|\/+$/g) into a local variable (e.g., trimmedBasepath) and then set
normalizedBasepath to '' if trimmedBasepath is empty or `/${trimmedBasepath}`
otherwise; ensure the final return uses normalizedBasepath so root and non-root
basepaths produce '/@tanstack-start/styles.css' and
'/my-app/@tanstack-start/styles.css' respectively.
🧹 Nitpick comments (2)
e2e/react-start/css-modules/tests/css.spec.ts (1)
28-33: HardenbuildUrlagainst non-leading-slash inputs.
Current call sites pass'/...', but the helper is easy to misuse later.Proposed diff
- const buildUrl = (baseURL: string, path: string) => { - return baseURL.replace(/\/$/, '') + path - } + const buildUrl = (baseURL: string, path: string) => { + const normalizedPath = path.startsWith('/') ? path : `/${path}` + return baseURL.replace(/\/$/, '') + normalizedPath + }packages/start-plugin-core/src/dev-server-plugin/plugin.ts (1)
70-75: Avoid(globalThis as any)forTSS_ROUTES_MANIFEST(type it once).
At least add a local type alias + narrow safely, or preferably declare a global type forTSS_ROUTES_MANIFESTto keep strictness. As per coding guidelines, keep TS type-safety high.
📜 Review details
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/css-modules/package.jsone2e/react-start/css-modules/playwright.config.tse2e/react-start/css-modules/tests/css.spec.tse2e/react-start/css-modules/vite.config.cloudflare.tse2e/react-start/css-modules/vite.config.tse2e/react-start/css-modules/wrangler.jsonce2e/solid-start/css-modules/package.jsone2e/vue-start/css-modules/package.jsonnx.jsonpackages/react-router/src/HeadContent.tsxpackages/router-core/src/index.tspackages/router-core/src/utils.tspackages/solid-router/src/HeadContent.tsxpackages/start-plugin-core/src/dev-server-plugin/dev-styles.tspackages/start-plugin-core/src/dev-server-plugin/plugin.tspackages/vue-router/src/HeadContent.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/router-core/src/utils.tspackages/router-core/src/index.tse2e/react-start/css-modules/tests/css.spec.tspackages/start-plugin-core/src/dev-server-plugin/dev-styles.tspackages/vue-router/src/HeadContent.tsxpackages/react-router/src/HeadContent.tsxe2e/react-start/css-modules/vite.config.tse2e/react-start/css-modules/vite.config.cloudflare.tspackages/start-plugin-core/src/dev-server-plugin/plugin.tspackages/solid-router/src/HeadContent.tsxe2e/react-start/css-modules/playwright.config.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/router-core/src/utils.tspackages/router-core/src/index.tse2e/react-start/css-modules/tests/css.spec.tspackages/start-plugin-core/src/dev-server-plugin/dev-styles.tspackages/vue-router/src/HeadContent.tsxpackages/react-router/src/HeadContent.tsxe2e/react-start/css-modules/vite.config.tse2e/react-start/css-modules/vite.config.cloudflare.tspackages/start-plugin-core/src/dev-server-plugin/plugin.tspackages/solid-router/src/HeadContent.tsxe2e/react-start/css-modules/playwright.config.ts
**/package.json
📄 CodeRabbit inference engine (AGENTS.md)
Use workspace protocol
workspace:*for internal dependencies in package.json files
Files:
e2e/vue-start/css-modules/package.jsone2e/solid-start/css-modules/package.jsone2e/react-start/css-modules/package.json
🧠 Learnings (12)
📓 Common learnings
Learnt from: nlynzaad
Repo: TanStack/router PR: 5182
File: e2e/react-router/basic-file-based/src/routes/non-nested/named/$baz_.bar.tsx:3-5
Timestamp: 2025-09-22T00:56:49.237Z
Learning: In TanStack Router, underscores are intentionally stripped from route segments (e.g., `$baz_` becomes `baz` in generated types) but should be preserved in base path segments. This is the correct behavior as of the fix in PR #5182.
Learnt from: nlynzaad
Repo: TanStack/router PR: 6215
File: e2e/react-start/custom-basepath/package.json:13-17
Timestamp: 2025-12-25T13:04:55.492Z
Learning: In the TanStack Router repository, e2e test scripts are specifically designed to run in CI (which uses a Unix environment), so Unix-specific commands (like `rm -rf`, `&` for backgrounding, and direct environment variable assignments without `cross-env`) are acceptable in e2e test npm scripts.
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Applies to **/package.json : Use workspace protocol `workspace:*` for internal dependencies in package.json files
📚 Learning: 2025-10-01T18:30:26.591Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.
Applied to files:
packages/router-core/src/utils.tspackages/router-core/src/index.tspackages/vue-router/src/HeadContent.tsxpackages/react-router/src/HeadContent.tsxpackages/solid-router/src/HeadContent.tsx
📚 Learning: 2025-12-17T02:17:55.086Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 6120
File: packages/router-generator/src/generator.ts:654-657
Timestamp: 2025-12-17T02:17:55.086Z
Learning: In `packages/router-generator/src/generator.ts`, pathless_layout routes must receive a `path` property when they have a `cleanedPath`, even though they are non-path routes. This is necessary because child routes inherit the path from their parent, and without this property, child routes would not have the correct full path at runtime.
Applied to files:
packages/router-core/src/utils.tspackages/router-core/src/index.tspackages/vue-router/src/HeadContent.tsxpackages/solid-router/src/HeadContent.tsx
📚 Learning: 2025-12-21T12:52:35.231Z
Learnt from: Sheraff
Repo: TanStack/router PR: 6171
File: packages/router-core/src/new-process-route-tree.ts:898-898
Timestamp: 2025-12-21T12:52:35.231Z
Learning: In `packages/router-core/src/new-process-route-tree.ts`, the matching logic intentionally allows paths without trailing slashes to match index routes with trailing slashes (e.g., `/a` can match `/a/` route), but not vice-versa (e.g., `/a/` cannot match `/a` layout route). This is implemented via the condition `!pathIsIndex || node.kind === SEGMENT_TYPE_INDEX` and is a deliberate design decision to provide better UX by being permissive with missing trailing slashes.
Applied to files:
packages/router-core/src/utils.ts
📚 Learning: 2025-10-09T12:59:02.129Z
Learnt from: hokkyss
Repo: TanStack/router PR: 5418
File: e2e/react-start/custom-identifier-prefix/src/styles/app.css:19-21
Timestamp: 2025-10-09T12:59:02.129Z
Learning: In e2e test directories (paths containing `e2e/`), accessibility concerns like outline suppression patterns are less critical since the code is for testing purposes, not production use.
Applied to files:
e2e/react-start/css-modules/tests/css.spec.ts
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
e2e/react-start/css-modules/tests/css.spec.tse2e/solid-start/css-modules/package.json
📚 Learning: 2025-10-09T12:59:14.842Z
Learnt from: hokkyss
Repo: TanStack/router PR: 5418
File: e2e/react-start/custom-identifier-prefix/public/site.webmanifest:2-3
Timestamp: 2025-10-09T12:59:14.842Z
Learning: In e2e test fixtures (files under e2e directories), empty or placeholder values in configuration files like site.webmanifest are acceptable and should not be flagged unless the test specifically validates those fields.
Applied to files:
e2e/react-start/css-modules/tests/css.spec.ts
📚 Learning: 2025-11-02T16:16:24.898Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5732
File: packages/start-client-core/src/client/hydrateStart.ts:6-9
Timestamp: 2025-11-02T16:16:24.898Z
Learning: In packages/start-client-core/src/client/hydrateStart.ts, the `import/no-duplicates` ESLint disable is necessary for imports from `#tanstack-router-entry` and `#tanstack-start-entry` because both aliases resolve to the same placeholder file (`fake-start-entry.js`) in package.json during static analysis, even though they resolve to different files at runtime.
Applied to files:
e2e/react-start/css-modules/tests/css.spec.tse2e/vue-start/css-modules/package.jsonpackages/vue-router/src/HeadContent.tsxpackages/react-router/src/HeadContent.tsxpackages/solid-router/src/HeadContent.tsxe2e/react-start/css-modules/wrangler.jsonce2e/solid-start/css-modules/package.jsone2e/react-start/css-modules/package.json
📚 Learning: 2025-12-25T13:04:55.492Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 6215
File: e2e/react-start/custom-basepath/package.json:13-17
Timestamp: 2025-12-25T13:04:55.492Z
Learning: In the TanStack Router repository, e2e test scripts are specifically designed to run in CI (which uses a Unix environment), so Unix-specific commands (like `rm -rf`, `&` for backgrounding, and direct environment variable assignments without `cross-env`) are acceptable in e2e test npm scripts.
Applied to files:
e2e/react-start/css-modules/tests/css.spec.tse2e/vue-start/css-modules/package.jsonnx.jsone2e/solid-start/css-modules/package.jsone2e/react-start/css-modules/playwright.config.tse2e/react-start/css-modules/package.json
📚 Learning: 2025-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Applies to **/package.json : Use workspace protocol `workspace:*` for internal dependencies in package.json files
Applied to files:
e2e/vue-start/css-modules/package.jsone2e/solid-start/css-modules/package.jsone2e/react-start/css-modules/package.json
📚 Learning: 2025-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Applies to **/*.{js,ts,tsx} : Implement ESLint rules for router best practices using the ESLint plugin router
Applied to files:
e2e/vue-start/css-modules/package.jsonpackages/vue-router/src/HeadContent.tsxpackages/solid-router/src/HeadContent.tsxe2e/solid-start/css-modules/package.json
📚 Learning: 2025-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Separate framework-agnostic core logic from React/Solid bindings
Applied to files:
packages/solid-router/src/HeadContent.tsx
🧬 Code graph analysis (5)
packages/router-core/src/utils.ts (2)
packages/router-core/src/index.ts (1)
buildDevStylesUrl(278-278)e2e/vue-start/custom-basepath/src/utils/basepath.ts (1)
basepath(1-1)
packages/react-router/src/HeadContent.tsx (1)
packages/router-core/src/utils.ts (1)
buildDevStylesUrl(612-620)
packages/start-plugin-core/src/dev-server-plugin/plugin.ts (1)
packages/start-plugin-core/src/dev-server-plugin/dev-styles.ts (1)
collectDevStyles(38-74)
packages/solid-router/src/HeadContent.tsx (1)
packages/router-core/src/utils.ts (1)
buildDevStylesUrl(612-620)
e2e/react-start/css-modules/playwright.config.ts (1)
examples/solid/quickstart-esbuild-file-based/build.js (1)
isDev(6-6)
⏰ 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 (16)
nx.json (1)
48-57: LGTM! Test target configuration is consistent.The new
test:e2e:nitroandtest:e2e:basepathtargets correctly mirror the existingtest:e2econfiguration pattern with appropriate caching, dependency chains, and input definitions.e2e/react-start/css-modules/vite.config.ts (2)
6-10: Clear documentation of environment variables.The comments effectively document the two configuration modes (basepath testing and Nitro server) that this config supports.
12-30: No other async Vite config patterns found; verify Nitro/Cloudflare Workers compatibility for this unique approach.The async
defineConfigfactory is valid and supported by Vite, but this is the only instance in the repository. While the pattern itself is technically sound for dynamic imports, ensure that Nitro, Cloudflare Workers, and test runners in your pipeline are tested with this async config pattern, since it's not an established pattern elsewhere in the codebase.e2e/solid-start/css-modules/package.json (1)
17-18: LGTM! Workspace protocol updated correctly.The change from
workspace:^toworkspace:*aligns with the repository's coding guidelines for internal dependencies.Based on coding guidelines, which specify using
workspace:*for all internal dependencies.Also applies to: 23-23
packages/start-plugin-core/src/dev-server-plugin/dev-styles.ts (1)
47-47: Correctly removed unnecessaryawaitfrom synchronous method.The
ModuleGraph.getModuleById()method is synchronous in Vite's API and returnsModuleNode | EnvironmentModuleNode | undefineddirectly. Removing theawaitkeywords corrects this usage.e2e/react-start/css-modules/vite.config.cloudflare.ts (1)
1-16: LGTM! Cloudflare configuration is clean and focused.The configuration appropriately sets up Cloudflare SSR with the required plugins. Disabling
inspectorPortis sensible for E2E testing to avoid port conflicts, andviteEnvironment: { name: 'ssr' }correctly configures the Vite environment naming for Workers.e2e/react-start/css-modules/playwright.config.ts (1)
6-16: Verify Playwright config runtime supports top-levelawait+import ... with { type: 'json' }in CI/dev.
If this config is ever loaded in a non-ESM context, it’ll hard-fail before tests start.e2e/vue-start/css-modules/package.json (1)
17-18: Good: internal dependencies useworkspace:*per repo convention.Also applies to: 23-23
e2e/react-start/css-modules/tests/css.spec.ts (1)
5-8: ThewhitelistErrorspattern here is already appropriately specific and scoped.The
/Failed to load resource: net::ERR_NAME_NOT_RESOLVED/regex is narrower than similar patterns used throughout the test suite (/404/,/500/,/499/), matching only DNS resolution failures. Since all tests in this describe block perform network requests (test.beforeAll,page.goto), scoping to individual tests would not provide practical benefit. The current approach at the describe level is consistent with best practices in the codebase.packages/react-router/src/HeadContent.tsx (1)
2-2: Good centralization, but basepath must match the middleware route.
UsingbuildDevStylesUrl(router.basepath, routeIds)is the right direction; please ensure the dev-server CSS middleware matches requests whenrouter.basepathis not/(and preserves underscores in basepath segments, as intended). Based on learnings, underscores should be preserved in basepath.Also applies to: 225-225
packages/vue-router/src/HeadContent.tsx (1)
3-3: LGTM; same basepath↔middleware alignment requirement applies.
This will only work end-to-end if the dev styles middleware accepts the basepath-prefixed.../@tanstack-start/styles.cssURL shape.Also applies to: 18-18, 30-32
packages/solid-router/src/HeadContent.tsx (1)
4-4: LGTM; verify basepath-prefixed dev-styles requests are handled.
Ifrouter.basepathcan be non-root, the server middleware must match that path (not just/@tanstack-start/styles.css).Also applies to: 207-207, 219-219
e2e/react-start/css-modules/package.json (4)
29-30: LGTM! Workspace protocol updated correctly.The change from
workspace:^toworkspace:*for internal TanStack dependencies aligns with the repository's coding guidelines.As per coding guidelines,
workspace:*should be used for internal dependencies in package.json files.Also applies to: 37-37
42-42: Verify using unpinned nightly version is intentional.The
nitro-nightly@latestdependency will always pull the most recent nightly build, which could introduce breaking changes and non-deterministic behavior in CI. For e2e tests, this might cause unexpected failures.Consider pinning to a specific nightly version (e.g.,
[email protected]) if stability is important, or confirm this unpinned approach is intentional for testing against the latest Nitro features.
9-12: LGTM! Script structure follows existing patterns.The new Nitro and basepath test variants follow the established e2e test script patterns. The use of Unix-specific commands (
rm -rf, direct environment variable assignments) is acceptable per repository standards.Based on learnings, Unix-specific commands are acceptable in e2e test scripts as they run in CI with a Unix environment.
Also applies to: 19-20, 23-25
13-14: Verify intentionally disabled Cloudflare dev scripts.The Cloudflare dev scripts are intentionally disabled with clear error messages about React duplication issues. However, there's an inconsistency in exit codes:
dev:cloudflareanddev:e2e:cloudflareexit with 1 (failure)_test:e2e:cloudflareexits with 0 (success) despite being disabledConfirm this is intentional. The underscore prefix and exit 0 in the test script suggest it's meant to pass in CI even when disabled, which seems reasonable, but worth verifying the approach is correct.
Also applies to: 26-26
| // When using basepath config, the app is served at /my-app | ||
| const basePath = viteConfig === 'basepath' ? '/my-app' : '' | ||
| const baseURL = `http://localhost:${PORT}${basePath}` | ||
|
|
||
| // Select the appropriate dev command based on VITE_CONFIG | ||
| const devCommand = viteConfig ? `pnpm dev:e2e:${viteConfig}` : 'pnpm dev:e2e' | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n e2e/react-start/css-modules/playwright.config.tsRepository: TanStack/router
Length of output: 1735
🏁 Script executed:
find . -name "package.json" -path "*/e2e/react-start/css-modules/*" -exec cat {} \;Repository: TanStack/router
Length of output: 2276
Ensure VITE_CONFIG environment variable is propagated to webServer.env for explicit contract clarity.
The baseURL and devCommand are both derived from process.env.VITE_CONFIG (lines 12, 16), creating an implicit contract that callers must set VITE_CONFIG consistently. However, VITE_CONFIG is not passed to webServer.env (lines 35–37), making this contract invisible to the spawned dev server. Add VITE_CONFIG to webServer.env to clarify the dependency and prevent accidental misconfiguration when running locally:
webServer.env block (lines 35–37)
env: {
VITE_NODE_ENV: 'test',
PORT: String(PORT),
VITE_CONFIG: viteConfig || '',
},
🤖 Prompt for AI Agents
In @e2e/react-start/css-modules/playwright.config.ts around lines 11 - 17, The
test harness builds baseURL and devCommand from process.env.VITE_CONFIG (via the
local viteConfig variable) but does not propagate that variable into the
Playwright webServer.env, making the dev server unaware of the same config;
update the webServer.env block in playwright.config.ts (the webServer
configuration object) to include VITE_CONFIG with the same value you compute for
viteConfig (e.g. VITE_CONFIG: viteConfig || ''), so the spawned dev server
receives the same VITE_CONFIG used to construct baseURL and devCommand.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @e2e/react-start/server-functions/tests/server-functions.spec.ts:
- Around line 740-742: Replace the overly-generic whitelist entry in the
test.describe block by changing test.use({ whitelistErrors: ['500'] }) to a
specific, anchored pattern that exactly matches the console error this test
expects (e.g. the full error string emitted by the unhandled middleware
exception) or a strict regex anchored with ^ and $; update the whitelistErrors
array in the test.use call so it uses that exact message or anchored regex
instead of the bare '500'.
In @e2e/solid-start/css-modules/playwright.config.ts:
- Line 5: The current isDev check (const isDev = process.env.MODE !== 'prod')
treats an undefined MODE as dev and causes production E2E tests to run the dev
server; change the logic to explicitly detect dev mode or default to prod by
reading MODE with a safe default and comparing for 'dev' (e.g., compute a mode
from process.env.MODE ?? 'prod' and set isDev = mode === 'dev'), and/or update
the test:e2e:prod npm scripts to set MODE=prod so production runs are explicit;
update the variable name isDev and any use site (webServer command selection) in
playwright.config.ts to use the new check.
🧹 Nitpick comments (2)
e2e/e2e-utils/src/fixture.ts (2)
19-24: Consider using a named parameter for clarity.The empty object pattern
async ({}, use)is flagged by static analysis. While technically valid (indicating no fixture dependencies), using a more explicit pattern improves readability.♻️ Proposed refactor for clarity
whitelistErrors: [ - async ({}, use) => { + async (_fixtures, use) => { await use([]) }, { option: true }, ],
27-28: Consider removing defensive fallback.The defensive check
Array.isArray(whitelistErrors) ? whitelistErrors : []may be unnecessary since the fixture provider above guarantees whitelistErrors is always an array. This fallback adds complexity without clear benefit.However, if you want to retain defensive programming for robustness against future changes, the current implementation is acceptable.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
e2e/e2e-utils/src/fixture.tse2e/react-start/basic/tests/navigation.spec.tse2e/react-start/basic/tests/not-found.spec.tse2e/react-start/basic/tests/open-redirect-prevention.spec.tse2e/react-start/basic/tests/params.spec.tse2e/react-start/css-modules/playwright.config.tse2e/react-start/css-modules/tests/css.spec.tse2e/react-start/serialization-adapters/tests/app.spec.tse2e/react-start/server-functions/tests/server-functions.spec.tse2e/solid-start/basic/tests/navigation.spec.tse2e/solid-start/basic/tests/not-found.spec.tse2e/solid-start/basic/tests/params.spec.tse2e/solid-start/css-modules/playwright.config.tse2e/solid-start/css-modules/tests/css.spec.tse2e/solid-start/serialization-adapters/tests/app.spec.tse2e/vue-start/basic/tests/navigation.spec.tse2e/vue-start/basic/tests/not-found.spec.tse2e/vue-start/basic/tests/params.spec.tse2e/vue-start/css-modules/playwright.config.tse2e/vue-start/css-modules/tests/css.spec.tse2e/vue-start/serialization-adapters/tests/app.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/react-start/css-modules/playwright.config.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
e2e/solid-start/serialization-adapters/tests/app.spec.tse2e/react-start/server-functions/tests/server-functions.spec.tse2e/solid-start/css-modules/tests/css.spec.tse2e/react-start/basic/tests/open-redirect-prevention.spec.tse2e/solid-start/basic/tests/navigation.spec.tse2e/react-start/basic/tests/params.spec.tse2e/vue-start/css-modules/tests/css.spec.tse2e/vue-start/css-modules/playwright.config.tse2e/solid-start/basic/tests/params.spec.tse2e/vue-start/basic/tests/params.spec.tse2e/vue-start/serialization-adapters/tests/app.spec.tse2e/vue-start/basic/tests/navigation.spec.tse2e/react-start/basic/tests/navigation.spec.tse2e/react-start/css-modules/tests/css.spec.tse2e/solid-start/basic/tests/not-found.spec.tse2e/react-start/serialization-adapters/tests/app.spec.tse2e/react-start/basic/tests/not-found.spec.tse2e/vue-start/basic/tests/not-found.spec.tse2e/solid-start/css-modules/playwright.config.tse2e/e2e-utils/src/fixture.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
e2e/solid-start/serialization-adapters/tests/app.spec.tse2e/react-start/server-functions/tests/server-functions.spec.tse2e/solid-start/css-modules/tests/css.spec.tse2e/react-start/basic/tests/open-redirect-prevention.spec.tse2e/solid-start/basic/tests/navigation.spec.tse2e/react-start/basic/tests/params.spec.tse2e/vue-start/css-modules/tests/css.spec.tse2e/vue-start/css-modules/playwright.config.tse2e/solid-start/basic/tests/params.spec.tse2e/vue-start/basic/tests/params.spec.tse2e/vue-start/serialization-adapters/tests/app.spec.tse2e/vue-start/basic/tests/navigation.spec.tse2e/react-start/basic/tests/navigation.spec.tse2e/react-start/css-modules/tests/css.spec.tse2e/solid-start/basic/tests/not-found.spec.tse2e/react-start/serialization-adapters/tests/app.spec.tse2e/react-start/basic/tests/not-found.spec.tse2e/vue-start/basic/tests/not-found.spec.tse2e/solid-start/css-modules/playwright.config.tse2e/e2e-utils/src/fixture.ts
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Applies to **/*.{js,ts,tsx} : Implement ESLint rules for router best practices using the ESLint plugin router
Learnt from: nlynzaad
Repo: TanStack/router PR: 6215
File: e2e/react-start/custom-basepath/package.json:13-17
Timestamp: 2025-12-25T13:04:55.492Z
Learning: In the TanStack Router repository, e2e test scripts are specifically designed to run in CI (which uses a Unix environment), so Unix-specific commands (like `rm -rf`, `&` for backgrounding, and direct environment variable assignments without `cross-env`) are acceptable in e2e test npm scripts.
Learnt from: schiller-manuel
Repo: TanStack/router PR: 6120
File: packages/router-generator/src/generator.ts:654-657
Timestamp: 2025-12-17T02:17:55.086Z
Learning: In `packages/router-generator/src/generator.ts`, pathless_layout routes must receive a `path` property when they have a `cleanedPath`, even though they are non-path routes. This is necessary because child routes inherit the path from their parent, and without this property, child routes would not have the correct full path at runtime.
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Applies to **/package.json : Use workspace protocol `workspace:*` for internal dependencies in package.json files
📚 Learning: 2025-10-09T12:59:14.842Z
Learnt from: hokkyss
Repo: TanStack/router PR: 5418
File: e2e/react-start/custom-identifier-prefix/public/site.webmanifest:2-3
Timestamp: 2025-10-09T12:59:14.842Z
Learning: In e2e test fixtures (files under e2e directories), empty or placeholder values in configuration files like site.webmanifest are acceptable and should not be flagged unless the test specifically validates those fields.
Applied to files:
e2e/solid-start/serialization-adapters/tests/app.spec.tse2e/react-start/server-functions/tests/server-functions.spec.tse2e/solid-start/css-modules/tests/css.spec.tse2e/react-start/basic/tests/open-redirect-prevention.spec.tse2e/solid-start/basic/tests/navigation.spec.tse2e/react-start/basic/tests/params.spec.tse2e/vue-start/css-modules/tests/css.spec.tse2e/vue-start/basic/tests/params.spec.tse2e/vue-start/serialization-adapters/tests/app.spec.tse2e/vue-start/basic/tests/navigation.spec.tse2e/react-start/basic/tests/navigation.spec.tse2e/react-start/css-modules/tests/css.spec.tse2e/solid-start/basic/tests/not-found.spec.tse2e/react-start/serialization-adapters/tests/app.spec.tse2e/react-start/basic/tests/not-found.spec.tse2e/vue-start/basic/tests/not-found.spec.tse2e/e2e-utils/src/fixture.ts
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
e2e/react-start/server-functions/tests/server-functions.spec.tse2e/react-start/basic/tests/open-redirect-prevention.spec.tse2e/solid-start/basic/tests/navigation.spec.tse2e/vue-start/css-modules/tests/css.spec.tse2e/vue-start/serialization-adapters/tests/app.spec.tse2e/vue-start/basic/tests/navigation.spec.tse2e/react-start/basic/tests/navigation.spec.tse2e/react-start/css-modules/tests/css.spec.tse2e/react-start/serialization-adapters/tests/app.spec.tse2e/vue-start/basic/tests/not-found.spec.ts
📚 Learning: 2025-12-25T13:04:55.492Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 6215
File: e2e/react-start/custom-basepath/package.json:13-17
Timestamp: 2025-12-25T13:04:55.492Z
Learning: In the TanStack Router repository, e2e test scripts are specifically designed to run in CI (which uses a Unix environment), so Unix-specific commands (like `rm -rf`, `&` for backgrounding, and direct environment variable assignments without `cross-env`) are acceptable in e2e test npm scripts.
Applied to files:
e2e/solid-start/css-modules/tests/css.spec.tse2e/vue-start/css-modules/tests/css.spec.tse2e/vue-start/css-modules/playwright.config.tse2e/react-start/css-modules/tests/css.spec.tse2e/solid-start/css-modules/playwright.config.ts
📚 Learning: 2025-10-01T18:30:26.591Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.
Applied to files:
e2e/react-start/basic/tests/open-redirect-prevention.spec.tse2e/react-start/css-modules/tests/css.spec.ts
📚 Learning: 2025-10-09T12:59:02.129Z
Learnt from: hokkyss
Repo: TanStack/router PR: 5418
File: e2e/react-start/custom-identifier-prefix/src/styles/app.css:19-21
Timestamp: 2025-10-09T12:59:02.129Z
Learning: In e2e test directories (paths containing `e2e/`), accessibility concerns like outline suppression patterns are less critical since the code is for testing purposes, not production use.
Applied to files:
e2e/react-start/basic/tests/open-redirect-prevention.spec.tse2e/solid-start/basic/tests/navigation.spec.tse2e/vue-start/css-modules/tests/css.spec.tse2e/react-start/basic/tests/navigation.spec.tse2e/react-start/css-modules/tests/css.spec.tse2e/solid-start/basic/tests/not-found.spec.tse2e/solid-start/css-modules/playwright.config.ts
📚 Learning: 2025-11-02T16:16:24.898Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5732
File: packages/start-client-core/src/client/hydrateStart.ts:6-9
Timestamp: 2025-11-02T16:16:24.898Z
Learning: In packages/start-client-core/src/client/hydrateStart.ts, the `import/no-duplicates` ESLint disable is necessary for imports from `#tanstack-router-entry` and `#tanstack-start-entry` because both aliases resolve to the same placeholder file (`fake-start-entry.js`) in package.json during static analysis, even though they resolve to different files at runtime.
Applied to files:
e2e/vue-start/serialization-adapters/tests/app.spec.tse2e/react-start/css-modules/tests/css.spec.tse2e/react-start/serialization-adapters/tests/app.spec.ts
📚 Learning: 2025-12-21T12:52:35.231Z
Learnt from: Sheraff
Repo: TanStack/router PR: 6171
File: packages/router-core/src/new-process-route-tree.ts:898-898
Timestamp: 2025-12-21T12:52:35.231Z
Learning: In `packages/router-core/src/new-process-route-tree.ts`, the matching logic intentionally allows paths without trailing slashes to match index routes with trailing slashes (e.g., `/a` can match `/a/` route), but not vice-versa (e.g., `/a/` cannot match `/a` layout route). This is implemented via the condition `!pathIsIndex || node.kind === SEGMENT_TYPE_INDEX` and is a deliberate design decision to provide better UX by being permissive with missing trailing slashes.
Applied to files:
e2e/react-start/css-modules/tests/css.spec.ts
🧬 Code graph analysis (4)
e2e/react-start/server-functions/tests/server-functions.spec.ts (1)
e2e/e2e-utils/src/fixture.ts (1)
test(18-47)
e2e/solid-start/css-modules/tests/css.spec.ts (1)
e2e/e2e-utils/src/fixture.ts (1)
test(18-47)
e2e/vue-start/css-modules/tests/css.spec.ts (1)
e2e/e2e-utils/src/fixture.ts (1)
test(18-47)
e2e/react-start/css-modules/tests/css.spec.ts (2)
e2e/e2e-utils/src/fixture.ts (1)
test(18-47)packages/router-core/src/route.ts (1)
path(1590-1592)
🪛 Biome (2.1.2)
e2e/e2e-utils/src/fixture.ts
[error] 20-20: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
⏰ 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 (23)
e2e/react-start/server-functions/tests/server-functions.spec.ts (1)
370-375: The whitelist string is appropriately specific.The error message
'Failed to load resource: the server responded with a status of 500'correctly targets browser network errors for this async validation test. While it could be brittle to browser message format changes, this specificity is appropriate for e2e testing.e2e/solid-start/serialization-adapters/tests/app.spec.ts (1)
42-46: Consistent with PR-wide error whitelist standardization.The change from RegExp to exact string for the 499 status error aligns with the broader pattern across all test files in this PR. Note that 499 is a non-standard HTTP status code (typically nginx-specific for client-closed requests), so ensure this exact message format is consistently produced in your test environment.
e2e/react-start/basic/tests/params.spec.ts (1)
9-13: LGTM—consistent with standardization effort.The error whitelist update matches the PR-wide pattern of switching to exact string matching for known intermittent errors.
e2e/solid-start/basic/tests/params.spec.ts (1)
9-13: LGTM—consistent with standardization effort.The error whitelist update matches the PR-wide pattern of switching to exact string matching.
e2e/vue-start/serialization-adapters/tests/app.spec.ts (1)
42-46: LGTM—consistent across all frameworks.The error whitelist update completes the standardization across React, Solid, and Vue test suites.
e2e/react-start/basic/tests/not-found.spec.ts (1)
10-13: The string literals inwhitelistErrorsuse substring matching (via.includes()) rather than exact matching. The fixture implementation ate2e/e2e-utils/src/fixture.tsexplicitly handles both RegExp and string patterns, usingtext.includes(whitelistError)for strings. This provides appropriate flexibility, and the error message format is consistent across all test files in the codebase. The concern about brittleness is not applicable.Likely an incorrect or invalid review comment.
e2e/vue-start/basic/tests/params.spec.ts (1)
9-13: LGTM: Simplified error whitelisting.The change from RegExp to string literal is clearer and sufficient for matching this specific 404 error message. This aligns with the PR-wide standardization of error whitelisting across test suites.
e2e/vue-start/basic/tests/not-found.spec.ts (1)
9-14: LGTM: Consistent error matching approach.The simplified string literal for the 404 error matches the standardization pattern applied across the test suite.
e2e/react-start/basic/tests/open-redirect-prevention.spec.ts (1)
4-8: LGTM: Simplified error whitelisting.Switching to string literal is appropriate for matching this specific 404 error and aligns with the standardization across the test suite.
e2e/react-start/serialization-adapters/tests/app.spec.ts (1)
42-46: LGTM: Consistent error matching for 499 status.The change to string literal follows the same pattern as other test files and is appropriate for matching this specific server error.
e2e/vue-start/css-modules/tests/css.spec.ts (1)
4-14: LGTM: Comprehensive error whitelisting for CI reliability.The addition of multiple error types (network errors and Vite 504 responses) appropriately addresses CI flakiness in CSS module tests. The inline comments clearly explain each error category, which aids maintainability.
e2e/react-start/css-modules/tests/css.spec.ts (3)
4-10: LGTM! Good CI resilience improvement.The whitelistErrors array appropriately handles transient network issues and Vite dependency optimization reloads that can occur in CI environments. The comments clearly document the purpose of each error pattern.
34-39: LGTM! The buildUrl helper correctly handles basePath scenarios.The helper correctly concatenates baseURL with paths, which is necessary because Playwright's
goto()with absolute paths (like'/modules') ignores the path portion of baseURL. The implementation properly removes trailing slashes to avoid double-slashes.
162-163: LGTM! Pattern-based URL matching improves test robustness.The glob pattern
'**/modules'and regex/\/($|\?)/for home navigation correctly handle scenarios with or without basePath prefixes (e.g.,/modulesvs/my-app/modules).Also applies to: 175-176
e2e/e2e-utils/src/fixture.ts (1)
4-16: LGTM! Excellent documentation.The JSDoc clearly explains the whitelistErrors option with practical examples showing both string and RegExp usage.
e2e/solid-start/basic/tests/navigation.spec.ts (1)
6-6: LGTM! String literal matching is more explicit.The change from RegExp to string literal for the 404 error message is appropriate. String matching is more efficient for exact substring matches and aligns with the fixture's support for both string and RegExp patterns. This pattern is consistently applied across multiple test files in this PR.
e2e/react-start/basic/tests/navigation.spec.ts (1)
7-7: LGTM! Consistent with fixture improvements.The change to string literal matching for the 404 error aligns with the whitelistErrors fixture enhancements and is consistent across the PR.
e2e/vue-start/css-modules/playwright.config.ts (1)
5-5: Fix isDev logic to correctly distinguish production tests.The current logic
isDev = process.env.MODE !== 'prod'incorrectly treatsMODEas development when undefined. This breaks thetest:e2e:prodnpm script, which doesn't setMODEbut expects production behavior (runningpnpm build && PORT=${PORT} pnpm start). With the new logic, undefined evaluates to true, causing the dev server to run instead.The condition should explicitly check for dev mode:
const isDev = process.env.MODE === 'dev'e2e/solid-start/basic/tests/not-found.spec.ts (1)
10-11: LGTM: Error whitelist updated to string format.The change from RegExp to string literal is supported by the test fixture and maintains equivalent behavior for this static error message.
e2e/solid-start/css-modules/tests/css.spec.ts (3)
4-10: LGTM: Well-documented error whitelisting.The whitelisted errors are clearly explained and appropriate for handling transient CI issues during Vite dependency optimization.
13-13: LGTM: Correct application of error whitelist.The
whitelistErrorsconfiguration is properly applied at the test suite level.
35-35: LGTM: Whitelist correctly propagated to nested suite.The
whitelistErrorsis correctly included alongsidejavaScriptEnabled: falsein the nested test suite configuration.e2e/vue-start/basic/tests/navigation.spec.ts (1)
4-6: LGTM: Error whitelist updated to string format.The change from RegExp to string literal maintains equivalent behavior for this static error message and aligns with the PR-wide standardization of error whitelisting.
| test.describe('unhandled exception in middleware (issue #5266)', () => { | ||
| // Whitelist the expected 500 error since this test verifies error handling | ||
| test.use({ whitelistErrors: [/500/] }) | ||
| test.use({ whitelistErrors: ['500'] }) |
There was a problem hiding this comment.
Whitelist string '500' is too generic and may match unintended errors.
The string '500' could inadvertently match various unrelated console errors such as "Processed 500 items", "Port 5000", "Timeout after 5000ms", or "Error code: 15009". This overly permissive pattern might hide legitimate test failures.
Consider using a more specific pattern consistent with line 373.
🔍 Suggested fix
- test.use({ whitelistErrors: ['500'] })
+ test.use({ whitelistErrors: ['Failed to load resource: the server responded with a status of 500'] })📝 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.describe('unhandled exception in middleware (issue #5266)', () => { | |
| // Whitelist the expected 500 error since this test verifies error handling | |
| test.use({ whitelistErrors: [/500/] }) | |
| test.use({ whitelistErrors: ['500'] }) | |
| test.describe('unhandled exception in middleware (issue #5266)', () => { | |
| // Whitelist the expected 500 error since this test verifies error handling | |
| test.use({ whitelistErrors: ['Failed to load resource: the server responded with a status of 500'] }) |
🤖 Prompt for AI Agents
In @e2e/react-start/server-functions/tests/server-functions.spec.ts around lines
740 - 742, Replace the overly-generic whitelist entry in the test.describe block
by changing test.use({ whitelistErrors: ['500'] }) to a specific, anchored
pattern that exactly matches the console error this test expects (e.g. the full
error string emitted by the unhandled middleware exception) or a strict regex
anchored with ^ and $; update the whitelistErrors array in the test.use call so
it uses that exact message or anchored regex instead of the bare '500'.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/solid-start/css-modules/tests/css.spec.ts (1)
37-167: Critical: Missing basePath support implementation.Unlike the React version (
e2e/react-start/css-modules/tests/css.spec.ts), this file is missing the complete basePath support implementation:
- Missing
buildUrlhelper: The React version definesbuildUrl(baseURL, path)at lines 34-39 to construct full URLs- Missing
baseURLparameter: Test callbacks don't receive thebaseURLparameter- Direct path navigation: Tests still use
page.goto('/')andpage.goto('/modules')instead ofbuildUrl(baseURL!, '/')This creates critical inconsistencies:
- Tests will fail when run with basePath configurations (e.g.,
/my-app)- The regex change at line 159 is insufficient without updating the navigation calls
- Behavior differs across framework test suites, undermining the PR's basePath support goal
🔧 Proposed fix to add basePath support
Add the buildUrl helper and update all navigation calls:
test.describe('CSS styles in SSR (dev mode)', () => { test.use({ whitelistErrors }) + // Helper to build full URL from baseURL and path + // Playwright's goto with absolute paths (like '/modules') ignores baseURL's path portion + // So we need to manually construct the full URL + const buildUrl = (baseURL: string, path: string) => { + return baseURL.replace(/\/$/, '') + path + } + test.describe('with JavaScript disabled', () => { test.use({ javaScriptEnabled: false, whitelistErrors }) - test('global CSS is applied on initial page load', async ({ page }) => { - await page.goto('/') + test('global CSS is applied on initial page load', async ({ + page, + baseURL, + }) => { + await page.goto(buildUrl(baseURL!, '/')) test('CSS modules are applied on initial page load', async ({ page, + baseURL, }) => { - await page.goto('/modules') + await page.goto(buildUrl(baseURL!, '/modules')) - test('global CSS class names are NOT scoped', async ({ page }) => { - await page.goto('/') + test('global CSS class names are NOT scoped', async ({ page, baseURL }) => { + await page.goto(buildUrl(baseURL!, '/')) }) - test('styles persist after hydration', async ({ page }) => { - await page.goto('/') + test('styles persist after hydration', async ({ page, baseURL }) => { + await page.goto(buildUrl(baseURL!, '/')) - test('CSS modules styles persist after hydration', async ({ page }) => { - await page.goto('/modules') + test('CSS modules styles persist after hydration', async ({ + page, + baseURL, + }) => { + await page.goto(buildUrl(baseURL!, '/modules')) test('styles work correctly after client-side navigation', async ({ page, + baseURL, }) => { // Start from home - await page.goto('/') + await page.goto(buildUrl(baseURL!, '/')) // Navigate to modules page await page.getByTestId('nav-modules').click() - await page.waitForURL('/modules') + // Use glob pattern to match with or without basepath + await page.waitForURL('**/modules')
🧹 Nitpick comments (2)
e2e/e2e-utils/src/fixture.ts (1)
28-29: Optional: Consider removing the redundant Array.isArray check.The defensive fallback on lines 28-29 appears unnecessary since:
- TypeScript types guarantee
whitelistErrorsisArray<RegExp | string>- The fixture now always provides an empty array by default (line 22)
- Tests using this fixture will pass arrays via
test.use()The check is harmless but adds minor complexity without clear benefit given the strong typing.
♻️ Optional simplification
- // Ensure whitelistErrors is always an array (defensive fallback) - const errors = Array.isArray(whitelistErrors) ? whitelistErrors : [] page.on('console', (m) => { if (m.type() === 'error') { const text = m.text() - for (const whitelistError of errors) { + for (const whitelistError of whitelistErrors) {Also applies to: 33-33
e2e/react-start/css-modules/tests/css.spec.ts (1)
34-39: Consider centralizing the buildUrl helper.The
buildUrlhelper is defined locally here and likely duplicated in other e2e test files (e.g., vue-start, solid-start if properly implemented). Consider extracting it to@tanstack/router-e2e-utilsto reduce duplication and ensure consistency.💡 Suggested centralized implementation
In
e2e/e2e-utils/src/fixture.ts, add:/** * Build full URL from baseURL and path. * Playwright's goto with absolute paths ignores baseURL's path portion, * so we manually construct the full URL to support basePath configurations. */ export function buildUrl(baseURL: string, path: string): string { return baseURL.replace(/\/$/, '') + path }Then import and use:
import { test, buildUrl } from '@tanstack/router-e2e-utils'
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
e2e/e2e-utils/src/fixture.tse2e/react-start/css-modules/tests/css.spec.tse2e/solid-start/css-modules/tests/css.spec.tse2e/vue-start/css-modules/tests/css.spec.tspackages/router-core/src/utils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/vue-start/css-modules/tests/css.spec.ts
- packages/router-core/src/utils.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
e2e/react-start/css-modules/tests/css.spec.tse2e/solid-start/css-modules/tests/css.spec.tse2e/e2e-utils/src/fixture.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
e2e/react-start/css-modules/tests/css.spec.tse2e/solid-start/css-modules/tests/css.spec.tse2e/e2e-utils/src/fixture.ts
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Applies to **/*.{js,ts,tsx} : Implement ESLint rules for router best practices using the ESLint plugin router
Learnt from: nlynzaad
Repo: TanStack/router PR: 6215
File: e2e/react-start/custom-basepath/package.json:13-17
Timestamp: 2025-12-25T13:04:55.492Z
Learning: In the TanStack Router repository, e2e test scripts are specifically designed to run in CI (which uses a Unix environment), so Unix-specific commands (like `rm -rf`, `&` for backgrounding, and direct environment variable assignments without `cross-env`) are acceptable in e2e test npm scripts.
Learnt from: schiller-manuel
Repo: TanStack/router PR: 6120
File: packages/router-generator/src/generator.ts:654-657
Timestamp: 2025-12-17T02:17:55.086Z
Learning: In `packages/router-generator/src/generator.ts`, pathless_layout routes must receive a `path` property when they have a `cleanedPath`, even though they are non-path routes. This is necessary because child routes inherit the path from their parent, and without this property, child routes would not have the correct full path at runtime.
📚 Learning: 2025-10-09T12:59:02.129Z
Learnt from: hokkyss
Repo: TanStack/router PR: 5418
File: e2e/react-start/custom-identifier-prefix/src/styles/app.css:19-21
Timestamp: 2025-10-09T12:59:02.129Z
Learning: In e2e test directories (paths containing `e2e/`), accessibility concerns like outline suppression patterns are less critical since the code is for testing purposes, not production use.
Applied to files:
e2e/react-start/css-modules/tests/css.spec.tse2e/solid-start/css-modules/tests/css.spec.ts
📚 Learning: 2025-10-09T12:59:14.842Z
Learnt from: hokkyss
Repo: TanStack/router PR: 5418
File: e2e/react-start/custom-identifier-prefix/public/site.webmanifest:2-3
Timestamp: 2025-10-09T12:59:14.842Z
Learning: In e2e test fixtures (files under e2e directories), empty or placeholder values in configuration files like site.webmanifest are acceptable and should not be flagged unless the test specifically validates those fields.
Applied to files:
e2e/react-start/css-modules/tests/css.spec.tse2e/solid-start/css-modules/tests/css.spec.tse2e/e2e-utils/src/fixture.ts
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
e2e/react-start/css-modules/tests/css.spec.tse2e/solid-start/css-modules/tests/css.spec.ts
📚 Learning: 2025-12-21T12:52:35.231Z
Learnt from: Sheraff
Repo: TanStack/router PR: 6171
File: packages/router-core/src/new-process-route-tree.ts:898-898
Timestamp: 2025-12-21T12:52:35.231Z
Learning: In `packages/router-core/src/new-process-route-tree.ts`, the matching logic intentionally allows paths without trailing slashes to match index routes with trailing slashes (e.g., `/a` can match `/a/` route), but not vice-versa (e.g., `/a/` cannot match `/a` layout route). This is implemented via the condition `!pathIsIndex || node.kind === SEGMENT_TYPE_INDEX` and is a deliberate design decision to provide better UX by being permissive with missing trailing slashes.
Applied to files:
e2e/react-start/css-modules/tests/css.spec.ts
📚 Learning: 2025-10-01T18:30:26.591Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.
Applied to files:
e2e/react-start/css-modules/tests/css.spec.ts
📚 Learning: 2025-12-25T13:04:55.492Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 6215
File: e2e/react-start/custom-basepath/package.json:13-17
Timestamp: 2025-12-25T13:04:55.492Z
Learning: In the TanStack Router repository, e2e test scripts are specifically designed to run in CI (which uses a Unix environment), so Unix-specific commands (like `rm -rf`, `&` for backgrounding, and direct environment variable assignments without `cross-env`) are acceptable in e2e test npm scripts.
Applied to files:
e2e/react-start/css-modules/tests/css.spec.tse2e/solid-start/css-modules/tests/css.spec.ts
🧬 Code graph analysis (2)
e2e/react-start/css-modules/tests/css.spec.ts (1)
e2e/e2e-utils/src/fixture.ts (1)
test(18-48)
e2e/solid-start/css-modules/tests/css.spec.ts (1)
e2e/e2e-utils/src/fixture.ts (1)
test(18-48)
🪛 Biome (2.1.2)
e2e/e2e-utils/src/fixture.ts
[error] 21-21: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
⏰ 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 (4)
e2e/e2e-utils/src/fixture.ts (2)
4-16: LGTM! Excellent documentation addition.The JSDoc clearly explains the purpose and usage of
whitelistErrorswith a practical example showing both string and RegExp patterns.
19-25: LGTM! Proper Playwright fixture pattern.The async fixture factory correctly follows Playwright's recommended pattern for options that can be configured via
test.use(). The empty object pattern on line 21 is standard Playwright syntax for fixtures without dependencies—the Biome error is a false positive and can be safely ignored.e2e/solid-start/css-modules/tests/css.spec.ts (1)
1-168: Inconsistent AI summary detected.The AI-generated summary claims this file introduces a
buildUrl(baseURL, path)helper and updates all tests to use baseURL-aware navigation, but the actual code shows these changes were NOT implemented in this file. The tests still use direct paths likepage.goto('/')without baseURL handling.e2e/react-start/css-modules/tests/css.spec.ts (1)
34-177: Excellent basePath support implementation.The changes consistently implement basePath support:
buildUrlhelper properly handles URL construction- All navigation calls updated to use
baseURLandbuildUrl- URL matching patterns updated to support basePath variations (
**/modulesglob, home regex)- Maintains test coverage while enabling flexible deployment paths
The implementation is clean and thorough.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
e2e/react-start/css-modules/playwright.config.ts (1)
7-7: Add type annotation for viteConfig.The viteConfig variable lacks a type annotation, which reduces type safety. Consider adding explicit typing to align with TypeScript strict mode guidelines.
🔧 Add type annotation
-const viteConfig = process.env.VITE_CONFIG // 'nitro' | 'basepath' | 'cloudflare' | undefined +const viteConfig = process.env.VITE_CONFIG as 'nitro' | 'basepath' | 'cloudflare' | undefinedOr for stricter validation:
-const viteConfig = process.env.VITE_CONFIG // 'nitro' | 'basepath' | 'cloudflare' | undefined +const validConfigs = ['nitro', 'basepath', 'cloudflare'] as const +const viteConfig: (typeof validConfigs)[number] | undefined = + validConfigs.includes(process.env.VITE_CONFIG as any) + ? (process.env.VITE_CONFIG as (typeof validConfigs)[number]) + : undefinedAs per coding guidelines, TypeScript strict mode with extensive type safety should be used for all code.
e2e/solid-start/css-modules/tests/css.spec.ts (1)
42-42: Consider removing redundant whitelistErrors.The
whitelistErrorsoption is already applied at the parent describe level (line 13), so including it again here may be redundant. However, if this is intentional for explicit clarity, it's harmless.♻️ Proposed simplification
- test.use({ javaScriptEnabled: false, whitelistErrors }) + test.use({ javaScriptEnabled: false })e2e/vue-start/css-modules/tests/css.spec.ts (1)
44-44: Remove redundant whitelistErrors.The whitelistErrors is already applied at the parent describe level (line 15), so reapplying it here is redundant.
♻️ Proposed fix
- test.use({ javaScriptEnabled: false, whitelistErrors }) + test.use({ javaScriptEnabled: false })
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
e2e/react-start/css-modules/playwright.config.tse2e/solid-start/css-modules/playwright.config.tse2e/solid-start/css-modules/tests/css.spec.tse2e/vue-start/css-modules/playwright.config.tse2e/vue-start/css-modules/tests/css.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/vue-start/css-modules/playwright.config.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
e2e/vue-start/css-modules/tests/css.spec.tse2e/react-start/css-modules/playwright.config.tse2e/solid-start/css-modules/tests/css.spec.tse2e/solid-start/css-modules/playwright.config.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
e2e/vue-start/css-modules/tests/css.spec.tse2e/react-start/css-modules/playwright.config.tse2e/solid-start/css-modules/tests/css.spec.tse2e/solid-start/css-modules/playwright.config.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Applies to **/*.{js,ts,tsx} : Implement ESLint rules for router best practices using the ESLint plugin router
Learnt from: nlynzaad
Repo: TanStack/router PR: 6215
File: e2e/react-start/custom-basepath/package.json:13-17
Timestamp: 2025-12-25T13:04:55.492Z
Learning: In the TanStack Router repository, e2e test scripts are specifically designed to run in CI (which uses a Unix environment), so Unix-specific commands (like `rm -rf`, `&` for backgrounding, and direct environment variable assignments without `cross-env`) are acceptable in e2e test npm scripts.
Learnt from: schiller-manuel
Repo: TanStack/router PR: 6120
File: packages/router-generator/src/generator.ts:654-657
Timestamp: 2025-12-17T02:17:55.086Z
Learning: In `packages/router-generator/src/generator.ts`, pathless_layout routes must receive a `path` property when they have a `cleanedPath`, even though they are non-path routes. This is necessary because child routes inherit the path from their parent, and without this property, child routes would not have the correct full path at runtime.
📚 Learning: 2025-10-09T12:59:02.129Z
Learnt from: hokkyss
Repo: TanStack/router PR: 5418
File: e2e/react-start/custom-identifier-prefix/src/styles/app.css:19-21
Timestamp: 2025-10-09T12:59:02.129Z
Learning: In e2e test directories (paths containing `e2e/`), accessibility concerns like outline suppression patterns are less critical since the code is for testing purposes, not production use.
Applied to files:
e2e/vue-start/css-modules/tests/css.spec.tse2e/solid-start/css-modules/tests/css.spec.ts
📚 Learning: 2025-12-25T13:04:55.492Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 6215
File: e2e/react-start/custom-basepath/package.json:13-17
Timestamp: 2025-12-25T13:04:55.492Z
Learning: In the TanStack Router repository, e2e test scripts are specifically designed to run in CI (which uses a Unix environment), so Unix-specific commands (like `rm -rf`, `&` for backgrounding, and direct environment variable assignments without `cross-env`) are acceptable in e2e test npm scripts.
Applied to files:
e2e/vue-start/css-modules/tests/css.spec.tse2e/react-start/css-modules/playwright.config.tse2e/solid-start/css-modules/tests/css.spec.tse2e/solid-start/css-modules/playwright.config.ts
📚 Learning: 2025-10-09T12:59:14.842Z
Learnt from: hokkyss
Repo: TanStack/router PR: 5418
File: e2e/react-start/custom-identifier-prefix/public/site.webmanifest:2-3
Timestamp: 2025-10-09T12:59:14.842Z
Learning: In e2e test fixtures (files under e2e directories), empty or placeholder values in configuration files like site.webmanifest are acceptable and should not be flagged unless the test specifically validates those fields.
Applied to files:
e2e/react-start/css-modules/playwright.config.tse2e/solid-start/css-modules/tests/css.spec.ts
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
e2e/solid-start/css-modules/tests/css.spec.ts
🧬 Code graph analysis (3)
e2e/vue-start/css-modules/tests/css.spec.ts (2)
e2e/e2e-utils/src/fixture.ts (1)
test(18-48)packages/router-core/src/route.ts (1)
path(1590-1592)
e2e/solid-start/css-modules/tests/css.spec.ts (1)
e2e/e2e-utils/src/fixture.ts (1)
test(18-48)
e2e/solid-start/css-modules/playwright.config.ts (2)
packages/start-plugin-core/src/start-compiler-plugin/compiler.ts (1)
mode(409-411)examples/solid/quickstart-esbuild-file-based/build.js (1)
isDev(6-6)
⏰ 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 (14)
e2e/solid-start/css-modules/playwright.config.ts (1)
5-6: LGTM! Explicit mode defaulting improves clarity.The two-step mode derivation with an explicit 'prod' default is clearer than the previous direct check and aligns with the pattern used across other e2e Playwright configs in this PR.
e2e/react-start/css-modules/playwright.config.ts (5)
5-6: LGTM! Consistent mode handling across e2e configs.The explicit mode defaulting matches the pattern in e2e/solid-start/css-modules/playwright.config.ts and improves clarity.
8-10: Good practice for avoiding port conflicts.Appending the viteConfig to the package name ensures different test configurations (nitro, basepath, cloudflare) can run concurrently without port conflicts.
12-14: LGTM! Correct basePath handling for deployment testing.The basePath logic correctly sets '/my-app' for the basepath configuration and updates baseURL accordingly, enabling proper testing of base-path deployments.
16-17: Good dynamic command selection.The devCommand construction appropriately selects the script based on viteConfig, enabling testing of different deployment scenarios (Nitro, basepath, Cloudflare).
32-32: LGTM! Proper integration of dynamic devCommand.The webServer configuration correctly uses the mode-aware devCommand for development, while maintaining the existing build-and-start flow for production.
e2e/solid-start/css-modules/tests/css.spec.ts (4)
4-10: LGTM: Well-defined error whitelist for CI resilience.The whitelistErrors array appropriately filters transient CI failures. The error patterns are specific and well-documented, aligning with similar patterns across other e2e suites in this PR.
34-39: LGTM: buildUrl helper correctly handles basepath scenarios.The helper correctly constructs absolute URLs by removing trailing slashes from baseURL before concatenation. The comment clearly explains why this is necessary for Playwright's goto behavior.
44-49: LGTM: Consistent basepath-aware navigation.All test functions have been consistently updated to accept
baseURLand use thebuildUrlhelper for navigation. The non-null assertions are appropriate for the Playwright test context.Also applies to: 71-75, 103-104, 115-116, 128-132, 144-149
163-163: LGTM: URL matching patterns correctly support basepaths.The glob pattern
**/modules(line 163) and the regex pattern for home navigation (lines 175-177) both correctly handle URLs with or without basepaths. The regex accurately matches the documented test cases including trailing slashes and query parameters.Also applies to: 175-177
e2e/vue-start/css-modules/tests/css.spec.ts (4)
4-15: LGTM! Well-structured error whitelisting for CI.The whitelistErrors array appropriately handles transient CI failures (network issues and Vite dependency optimization), and correctly integrates with the e2e-utils fixture to filter console errors during tests.
36-41: LGTM! Clear and correct URL construction.The buildUrl helper correctly handles basepath scenarios by properly concatenating baseURL and path. The explanatory comment is helpful.
46-151: LGTM! Consistent basepath support across all tests.All tests correctly accept baseURL and use buildUrl for navigation, enabling proper basepath support. The implementation is consistent and correct.
164-165: LGTM! Correct glob pattern for basepath support.The
**/modulesglob pattern correctly matches the modules route regardless of basepath prefix, which is the right approach for this basepath-aware test suite.
| // Match home URL with or without trailing slash and optional query string | ||
| // Matches: /, /?, /my-app, /my-app/, /my-app?foo=bar | ||
| await page.waitForURL(/\/([^/]*)(\/)?($|\?)/) |
There was a problem hiding this comment.
Fix overly broad regex that matches unintended routes.
The regex /\/([^/]*)(\/)?($|\?)/ is too permissive and will match /modules in addition to the intended home routes. When navigating back from the modules page, this could cause the test to pass prematurely while still on the modules page.
The pattern ([^/]*) matches ANY single path segment, including "modules".
🔧 Proposed fix options
Option 1: Use a glob pattern similar to line 165
- // Match home URL with or without trailing slash and optional query string
- // Matches: /, /?, /my-app, /my-app/, /my-app?foo=bar
- await page.waitForURL(/\/([^/]*)(\/)?($|\?)/)
+ // Match home URL - use glob to exclude other routes
+ await page.waitForURL('**/')Option 2: Explicitly exclude known routes
- await page.waitForURL(/\/([^/]*)(\/)?($|\?)/)
+ // Match home but not other routes like /modules
+ await page.waitForURL((url) => {
+ const pathname = new URL(url).pathname
+ return !pathname.includes('/modules')
+ })Option 3: More specific regex that only matches root or basepath
- await page.waitForURL(/\/([^/]*)(\/)?($|\?)/)
+ // Match only root or single-segment basepath, not deeper routes
+ await page.waitForURL(/^[^?]*\/([\w-]+\/)?\??.*$/)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @e2e/solid-start/css-modules/tests/css.spec.ts:
- Around line 7-10: The test introduces an inconsistent whitelistErrors array
(variable whitelistErrors in css.spec.ts) across frameworks; decide whether to
normalize or document: either add 'Failed to load resource:
net::ERR_INTERNET_DISCONNECTED' to the whitelistErrors in the Solid and React
css.spec.ts tests to match Vue, or add a comment in the Vue test explaining this
is a Vue CI-specific transient network error and remove it from the shared
expectations so frameworks remain consistent; update the whitelistErrors
variable and/or add the explanatory comment accordingly and run the e2e tests to
confirm behavior.
- Around line 37-42: The duplicated buildUrl helper should be extracted to a
shared utility and imported from there: add an exported function named
buildUrl(baseURL: string, path: string): string in the shared e2e utils module
(export const buildUrl = ...) and replace the local helper in each CSS module
test file (Solid/React/Vue) with an import of that shared buildUrl, removing the
local definitions so all tests use the single exported function.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
e2e/react-start/css-modules/tests/css.spec.tse2e/solid-start/css-modules/tests/css.spec.tse2e/vue-start/css-modules/tests/css.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
e2e/solid-start/css-modules/tests/css.spec.tse2e/react-start/css-modules/tests/css.spec.tse2e/vue-start/css-modules/tests/css.spec.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
e2e/solid-start/css-modules/tests/css.spec.tse2e/react-start/css-modules/tests/css.spec.tse2e/vue-start/css-modules/tests/css.spec.ts
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Applies to **/*.{js,ts,tsx} : Implement ESLint rules for router best practices using the ESLint plugin router
Learnt from: nlynzaad
Repo: TanStack/router PR: 6215
File: e2e/react-start/custom-basepath/package.json:13-17
Timestamp: 2025-12-25T13:04:55.492Z
Learning: In the TanStack Router repository, e2e test scripts are specifically designed to run in CI (which uses a Unix environment), so Unix-specific commands (like `rm -rf`, `&` for backgrounding, and direct environment variable assignments without `cross-env`) are acceptable in e2e test npm scripts.
Learnt from: schiller-manuel
Repo: TanStack/router PR: 6120
File: packages/router-generator/src/generator.ts:654-657
Timestamp: 2025-12-17T02:17:55.086Z
Learning: In `packages/router-generator/src/generator.ts`, pathless_layout routes must receive a `path` property when they have a `cleanedPath`, even though they are non-path routes. This is necessary because child routes inherit the path from their parent, and without this property, child routes would not have the correct full path at runtime.
📚 Learning: 2025-10-09T12:59:02.129Z
Learnt from: hokkyss
Repo: TanStack/router PR: 5418
File: e2e/react-start/custom-identifier-prefix/src/styles/app.css:19-21
Timestamp: 2025-10-09T12:59:02.129Z
Learning: In e2e test directories (paths containing `e2e/`), accessibility concerns like outline suppression patterns are less critical since the code is for testing purposes, not production use.
Applied to files:
e2e/solid-start/css-modules/tests/css.spec.tse2e/react-start/css-modules/tests/css.spec.tse2e/vue-start/css-modules/tests/css.spec.ts
📚 Learning: 2025-11-02T16:16:24.898Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5732
File: packages/start-client-core/src/client/hydrateStart.ts:6-9
Timestamp: 2025-11-02T16:16:24.898Z
Learning: In packages/start-client-core/src/client/hydrateStart.ts, the `import/no-duplicates` ESLint disable is necessary for imports from `#tanstack-router-entry` and `#tanstack-start-entry` because both aliases resolve to the same placeholder file (`fake-start-entry.js`) in package.json during static analysis, even though they resolve to different files at runtime.
Applied to files:
e2e/solid-start/css-modules/tests/css.spec.tse2e/react-start/css-modules/tests/css.spec.tse2e/vue-start/css-modules/tests/css.spec.ts
📚 Learning: 2025-10-09T12:59:14.842Z
Learnt from: hokkyss
Repo: TanStack/router PR: 5418
File: e2e/react-start/custom-identifier-prefix/public/site.webmanifest:2-3
Timestamp: 2025-10-09T12:59:14.842Z
Learning: In e2e test fixtures (files under e2e directories), empty or placeholder values in configuration files like site.webmanifest are acceptable and should not be flagged unless the test specifically validates those fields.
Applied to files:
e2e/solid-start/css-modules/tests/css.spec.ts
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
e2e/solid-start/css-modules/tests/css.spec.tse2e/vue-start/css-modules/tests/css.spec.ts
📚 Learning: 2025-12-25T13:04:55.492Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 6215
File: e2e/react-start/custom-basepath/package.json:13-17
Timestamp: 2025-12-25T13:04:55.492Z
Learning: In the TanStack Router repository, e2e test scripts are specifically designed to run in CI (which uses a Unix environment), so Unix-specific commands (like `rm -rf`, `&` for backgrounding, and direct environment variable assignments without `cross-env`) are acceptable in e2e test npm scripts.
Applied to files:
e2e/solid-start/css-modules/tests/css.spec.tse2e/react-start/css-modules/tests/css.spec.tse2e/vue-start/css-modules/tests/css.spec.ts
📚 Learning: 2025-12-21T12:52:35.231Z
Learnt from: Sheraff
Repo: TanStack/router PR: 6171
File: packages/router-core/src/new-process-route-tree.ts:898-898
Timestamp: 2025-12-21T12:52:35.231Z
Learning: In `packages/router-core/src/new-process-route-tree.ts`, the matching logic intentionally allows paths without trailing slashes to match index routes with trailing slashes (e.g., `/a` can match `/a/` route), but not vice-versa (e.g., `/a/` cannot match `/a` layout route). This is implemented via the condition `!pathIsIndex || node.kind === SEGMENT_TYPE_INDEX` and is a deliberate design decision to provide better UX by being permissive with missing trailing slashes.
Applied to files:
e2e/react-start/css-modules/tests/css.spec.tse2e/vue-start/css-modules/tests/css.spec.ts
📚 Learning: 2025-10-01T18:30:26.591Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.
Applied to files:
e2e/react-start/css-modules/tests/css.spec.tse2e/vue-start/css-modules/tests/css.spec.ts
📚 Learning: 2025-09-28T21:41:45.233Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5284
File: e2e/react-start/basic/server.js:50-0
Timestamp: 2025-09-28T21:41:45.233Z
Learning: In Express v5, catch-all routes must use named wildcards. Use `/*splat` to match everything except root path, or `/{*splat}` (with braces) to match including root path. The old `*` syntax is not allowed and will cause "Missing parameter name" errors. This breaking change requires explicit naming of wildcard parameters.
Applied to files:
e2e/vue-start/css-modules/tests/css.spec.ts
🧬 Code graph analysis (3)
e2e/solid-start/css-modules/tests/css.spec.ts (1)
e2e/e2e-utils/src/fixture.ts (1)
test(18-48)
e2e/react-start/css-modules/tests/css.spec.ts (1)
e2e/e2e-utils/src/fixture.ts (1)
test(18-48)
e2e/vue-start/css-modules/tests/css.spec.ts (1)
e2e/e2e-utils/src/fixture.ts (1)
test(18-48)
⏰ 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 (2)
e2e/solid-start/css-modules/tests/css.spec.ts (1)
184-186: Verify the home URL regex pattern covers all expected cases.The regex
/\/([^/]*)(\/)?($|\?)/is intended to match various home URL patterns, but it's complex and may not behave as expected. The pattern would match paths like/foo/bar(which isn't a home URL) because[^/]*matches any non-slash characters.Consider simplifying to explicitly match the expected patterns:
- Root:
/,/?...- With basepath:
/basepath,/basepath/,/basepath?...A more precise pattern might be:
/^\/[^\/]*\/?(\?.*)?$/However, verify the actual URLs being tested to ensure the pattern is correct.
e2e/vue-start/css-modules/tests/css.spec.ts (1)
8-12: Vue includes an additional whitelisted error.This file whitelists
net::ERR_INTERNET_DISCONNECTEDin addition to the two errors whitelisted in Solid and React. The verification script in the Solid review will help confirm whether this difference is intentional.
| const whitelistErrors = [ | ||
| 'Failed to load resource: net::ERR_NAME_NOT_RESOLVED', | ||
| 'Failed to load resource: the server responded with a status of 504', | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare whitelistErrors across all three CSS module test files
echo "=== Solid Start ==="
rg -A 5 "const whitelistErrors" e2e/solid-start/css-modules/tests/css.spec.ts
echo -e "\n=== React Start ==="
rg -A 5 "const whitelistErrors" e2e/react-start/css-modules/tests/css.spec.ts
echo -e "\n=== Vue Start ==="
rg -A 5 "const whitelistErrors" e2e/vue-start/css-modules/tests/css.spec.tsRepository: TanStack/router
Length of output: 761
Explain or resolve the whitelistErrors inconsistency between frameworks.
Vue's CSS module test whitelists net::ERR_INTERNET_DISCONNECTED, while Solid and React don't. Clarify whether this is a Vue-specific CI environment issue that should be documented, or if the three frameworks should have consistent error handling.
🤖 Prompt for AI Agents
In @e2e/solid-start/css-modules/tests/css.spec.ts around lines 7 - 10, The test
introduces an inconsistent whitelistErrors array (variable whitelistErrors in
css.spec.ts) across frameworks; decide whether to normalize or document: either
add 'Failed to load resource: net::ERR_INTERNET_DISCONNECTED' to the
whitelistErrors in the Solid and React css.spec.ts tests to match Vue, or add a
comment in the Vue test explaining this is a Vue CI-specific transient network
error and remove it from the shared expectations so frameworks remain
consistent; update the whitelistErrors variable and/or add the explanatory
comment accordingly and run the e2e tests to confirm behavior.
| // Helper to build full URL from baseURL and path | ||
| // Playwright's goto with absolute paths (like '/modules') ignores baseURL's path portion | ||
| // So we need to manually construct the full URL | ||
| const buildUrl = (baseURL: string, path: string) => { | ||
| return baseURL.replace(/\/$/, '') + path | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract the duplicated buildUrl helper to a shared utility.
This helper is duplicated identically across all three CSS module test files (Solid, React, and Vue). Extract it to @tanstack/router-e2e-utils to eliminate duplication and ensure consistency.
♻️ Suggested refactor
In e2e/e2e-utils/src/fixture.ts, add the helper:
export const buildUrl = (baseURL: string, path: string): string => {
return baseURL.replace(/\/$/, '') + path
}Then import it in each test file:
-import { test } from '@tanstack/router-e2e-utils'
+import { test, buildUrl } from '@tanstack/router-e2e-utils'And remove the local helper definition from all three files.
🤖 Prompt for AI Agents
In @e2e/solid-start/css-modules/tests/css.spec.ts around lines 37 - 42, The
duplicated buildUrl helper should be extracted to a shared utility and imported
from there: add an exported function named buildUrl(baseURL: string, path:
string): string in the shared e2e utils module (export const buildUrl = ...) and
replace the local helper in each CSS module test file (Solid/React/Vue) with an
import of that shared buildUrl, removing the local definitions so all tests use
the single exported function.
Summary by CodeRabbit
New Features
Refactor
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.