chore: unify react-start basic e2e mode projects#7206
chore: unify react-start basic e2e mode projects#7206beaussan merged 11 commits intoTanStack:mainfrom
Conversation
📝 WalkthroughWalkthroughE2E test infrastructure is consolidated by removing mode-specific test packages across React/Vue/Solid Start (prerender, preview, spa variants), consolidating functionality into primary "basic" packages with environment-driven mode configuration, and refactoring the Nx Playwright plugin to support metadata-based mode targeting. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes The changes involve substantial architectural refactoring across three frameworks with heterogeneous modifications: environment-driven configuration logic, Nx plugin rewrite with mode-based targeting, port management changes distributed across multiple files, test setup movement, and server configuration updates. High logic density in the plugin (~260 lines) and multiple interdependent changes across test packages require careful verification of mode-branching logic and port key derivation throughout. Possibly related PRs
Suggested labels
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit 5ccba2a
☁️ Nx Cloud last updated this comment at |
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Rename playwright mode metadata from bundler to toolchain, hardcode inferred build commands to 'vite build && tsc --noEmit', and drop the explicit rename-check error path while keeping only vite supported for now.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (8)
e2e/react-start/basic/tests/utils/getE2EPortKey.ts (1)
1-5: Duplicated helper across e2e suites.This file is byte-identical to
e2e/vue-start/basic-test-suite/src/utils/getE2EPortKey.ts(and presumably the Solid variant). Consider hoistinggetE2EPortKey(andgetPackageName) into@tanstack/router-e2e-utilsso all e2e suites share a single source of truth. Not blocking for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/react-start/basic/tests/utils/getE2EPortKey.ts` around lines 1 - 5, The getE2EPortKey helper is duplicated across e2e suites; extract getE2EPortKey (and its dependency getPackageName) into the shared package `@tanstack/router-e2e-utils` and update callers to import from that package instead of the local path; specifically, move the implementations for getE2EPortKey() and getPackageName() into the shared module, export them, then replace the local import in the file that currently imports './getPackageName.ts' with an import from '@tanstack/router-e2e-utils' and ensure getE2EPortKey() continues to return process.env.E2E_PORT_KEY ?? getPackageName().e2e/react-start/basic/tests/prerendering.spec.ts (1)
7-11: Consider resolvingdistDirrelative to the spec file rather thanprocess.cwd().Using
process.cwd()works because Playwright is launched frome2e/react-start/basic(perplaywright.config.ts), but it ties the test to the invocation directory. UsingfileURLToPath(import.meta.url)+..would make the path robust if a test is ever run from a different cwd.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/react-start/basic/tests/prerendering.spec.ts` around lines 7 - 11, The distDir is currently built from process.cwd(), which couples the test to the invocation directory; change distDir to resolve relative to the spec file by using fileURLToPath(import.meta.url) and dirname (e.g., import { fileURLToPath } from 'url' and import { dirname, join } from 'path') and compute distDir via join(dirname(fileURLToPath(import.meta.url)), '..', process.env.E2E_DIST_DIR ?? 'dist', 'client') so the variable distDir (used in this spec) is robust regardless of the current working directory.e2e/solid-start/basic/playwright.config.ts (1)
13-21: Duplicate port-file cleanup block with vue-start (and similarly shaped in react-start).This exact block —
TEST_WORKER_INDEXguard, list of threeport-${key}*.txtfiles,fs.rmSync(..., { force: true })— is repeated verbatim ine2e/vue-start/basic/playwright.config.ts(Lines 13–21). Given the PR already consolidates on shared E2E infrastructure, consider exposing a helper from@tanstack/router-e2e-utils, e.g.cleanupPortFiles(e2ePortKey), so the suffix convention (.txt,_start.txt,-external.txt) stays in one place next togetTestServerPort/getDummyServerPort.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/solid-start/basic/playwright.config.ts` around lines 13 - 21, Extract the repeated port-file cleanup into a shared helper (e.g. export function cleanupPortFiles(e2ePortKey: string)) in the `@tanstack/router-e2e-utils` module alongside getTestServerPort/getDummyServerPort; the helper should perform the TEST_WORKER_INDEX guard and call fs.rmSync on `port-${e2ePortKey}.txt`, `port-${e2ePortKey}_start.txt`, and `port-${e2ePortKey}-external.txt` with { force: true }. Then replace the duplicated block in playwright.config.ts (and the matching blocks in vue-start/react-start) with a call to cleanupPortFiles(e2ePortKey). Ensure the helper is exported and imported where used.scripts/nx/playwright-plugin.md (2)
146-160: Doc example missing the_startport file.The actual
e2e/react-start/basic/playwright.config.tscleanup loop also removes`port-${e2ePortKey}_start.txt`(driven bygetTestServerPort(${e2ePortKey}_start)), but the example here lists only the base and-externalvariants. Worth adding the_startentry so projects copying this snippet match the canonical config and avoid stale start-port files.📝 Suggested update
for (const portFile of [ `port-${e2ePortKey}.txt`, + `port-${e2ePortKey}_start.txt`, `port-${e2ePortKey}-external.txt`, ]) { fs.rmSync(portFile, { force: true }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/nx/playwright-plugin.md` around lines 146 - 160, The cleanup loop that removes stale port files (the fs.rmSync loop) is missing the `_start` variant; update the array of portFile names used with e2ePortKey so it includes `port-${e2ePortKey}_start.txt` alongside `port-${e2ePortKey}.txt` and `port-${e2ePortKey}-external.txt` (the code using e2ePortKey and fs.rmSync should remove the `_start` file to match getTestServerPort(`${e2ePortKey}_start`) behavior).
60-73: Clarify glob-like target notation.
build:e2e:*/test:e2e:*here uses:*as a wildcard, but the actual generated target names use--<toolchain>-<mode>(e.g.build:e2e--vite-ssr). Readers skimming this section may parsebuild:e2e:*as literal target segments. Considerbuild:e2e--*/test:e2e--*(or "each inferred build:e2e--… and test:e2e--… target") for parity with §2 and §7.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/nx/playwright-plugin.md` around lines 60 - 73, Update the glob-like target notation to match actual generated target names: replace occurrences of "build:e2e:*" and "test:e2e:*" with "build:e2e--*" and "test:e2e--*" (or add parenthetical text "each inferred build:e2e--… and test:e2e--… target") so the examples and prose match the real targets (e.g. build:e2e--vite-ssr); adjust the sentence that lists injected env vars (the block referencing MODE, TOOLCHAIN, E2E_TOOLCHAIN, E2E_DIST, E2E_DIST_DIR, and E2E_PORT_KEY) to use the corrected target notation.scripts/nx/playwright-plugin.ts (3)
192-225: Stale error prefix[Playwright Sharding Plugin].These errors originate from mode/toolchain validation, not sharding. Since the plugin now also infers mode targets, consider relaxing the prefix to something like
[Playwright Plugin]for the mode-related messages so the surface area in user-facing errors matches the new responsibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/nx/playwright-plugin.ts` around lines 192 - 225, In validateModeMetadata, change the error message prefix from "[Playwright Sharding Plugin]" to a more general "[Playwright Plugin]" for all mode/toolchain validation errors thrown using modeMetadata and index (the three throws that call isPlaywrightToolchain, isPlaywrightMode, and the missing toolchain/mode check); update the messages that reference PLAYWRIGHT_TOOLCHAINS and PLAYWRIGHT_MODES so the only change is the prefix text while leaving the rest of each string intact.
201-235:normalizeShardCountresult is discarded; recomputed later.
validateModeMetadatacallsnormalizeShardCountat line 228 purely for its throwing side effect, then returns the rawmodeMetadata.shards.buildModeTargetsthen re-runsnormalizeShardCountat line 259 to actually use the value. Cleaner to return the normalized count from validation and let callers use it directly — fewer redundant calls and fewer places that have to remember to re-normalize.♻️ Suggested refactor
-function validateModeMetadata( - modeMetadata: RawPlaywrightModeMetadata, - index: number, -): PlaywrightModeMetadata { +function validateModeMetadata( + modeMetadata: RawPlaywrightModeMetadata, + index: number, +): PlaywrightModeMetadata { const targetName = `${modeMetadata.toolchain ?? '<missing toolchain>'}:${modeMetadata.mode ?? '<missing mode>'}` ... - normalizeShardCount(modeMetadata.shards, targetName) - - return { - toolchain: modeMetadata.toolchain, - mode: modeMetadata.mode, - shards: modeMetadata.shards, - } + return { + toolchain: modeMetadata.toolchain, + mode: modeMetadata.mode, + shards: normalizeShardCount(modeMetadata.shards, targetName), + } }Then in
buildModeTargets, drop the second call:- const shardCount = normalizeShardCount(modeMetadata.shards, modeTargetName) + const shardCount = modeMetadata.shards ?? 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/nx/playwright-plugin.ts` around lines 201 - 235, validateModeMetadata currently calls normalizeShardCount only for its side effects and returns the original modeMetadata.shards, causing callers (buildModeTargets) to re-run normalization; change validateModeMetadata to capture and return the normalized shard count (call normalizeShardCount(modeMetadata.shards, targetName) and use its result in the returned PlaywrightModeMetadata.shards) and then update buildModeTargets to use the returned normalized shards instead of calling normalizeShardCount again; update any type annotations for PlaywrightModeMetadata.shards if needed so the normalized value is propagated rather than recomputed.
237-270: Silently overwrites duplicate{toolchain, mode}entries.If a
package.jsonaccidentally lists the same{toolchain, mode}pair twice (e.g. twovite/spaentries with differentshards), the second iteration overwrites the first intargets/targetGroupwith no warning. Consider tracking seen pairs and throwing invalidateModeMetadata(or here in the loop) to surface configuration errors early, consistent with the other validations.♻️ Suggested guard
const targets: Record<string, TargetConfiguration> = {} const targetGroup: Array<string> = [] + const seenModeKeys = new Set<string>() ... for (const [index, rawModeMetadata] of modes.entries()) { const modeMetadata = validateModeMetadata(rawModeMetadata, index) + const modeKey = `${modeMetadata.toolchain}:${modeMetadata.mode}` + if (seenModeKeys.has(modeKey)) { + throw new Error( + `[Playwright Plugin] Duplicate playwrightModes entry for ${modeKey} at index ${index}.`, + ) + } + seenModeKeys.add(modeKey) const modeTargetName = getModeTargetName(modeMetadata)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/nx/playwright-plugin.ts` around lines 237 - 270, buildModeTargets silently overwrites duplicate {toolchain, mode} entries when iterating modes; add a uniqueness guard (either in validateModeMetadata or at start of the loop in buildModeTargets) that tracks seen keys (e.g. derive a unique key from getModeTargetName(modeMetadata) or from modeMetadata.toolchain+':' + modeMetadata.mode) and throws a clear error if a duplicate is encountered so targets and targetGroup cannot be silently overwritten; update validateModeMetadata or the loop to perform this check before inserting into targets/targetGroup.
🤖 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/basic/package.json`:
- Line 14: The test:e2e:local npm script now only runs Playwright and can serve
stale/missing artifacts if a matching Nx build target (e.g.,
build:e2e--vite-prerender) wasn’t run first; update package.json to add
mode-aware convenience scripts or change test:e2e:local to run the appropriate
build+test sequence: detect MODE/E2E_DIST_DIR and invoke the corresponding Nx
build target (for example build:e2e--vite-prerender) before running "playwright
test --project=chromium", or add explicit test:e2e:local:<mode> aliases that run
"pnpm nx run <mode-specific-build> && pnpm test:e2e:local"; ensure any added
script preserves the existing Playwright cleanup behavior (playwright.config.ts
uses fs.rmSync) so port-file/dist cleanup still happens.
In `@e2e/react-start/basic/server.js`:
- Around line 12-17: Change the distDir assignment to use nullish coalescing
(distDir = process.env.E2E_DIST_DIR ?? 'dist'), and convert the filesystem path
before dynamic import: replace the raw import(distServerEntryPath) in
createStartServer with an import of a file:// URL created via
pathToFileURL(distServerEntryPath).toString() (ensure pathToFileURL is imported
from 'url'); apply the same pattern for distClientDir/distServerEntryPath usage
in createStartServer (and replicate these changes in the sibling files for
solid-start and vue-start).
In `@e2e/solid-start/basic/playwright.config.ts`:
- Around line 27-30: The preview branch of the command string (variable
commandByMode) only serves distDir and can run against an empty build when
invoked locally; update commandByMode so the preview branch prepends a build
step (e.g., run the appropriate pnpm build command before `pnpm preview --outDir
${distDir} --port ${PORT}`) so it is self-sufficient outside Nx, and ensure any
references to distDir/PORT remain unchanged; alternatively document this
requirement if you prefer not to change commandByMode.
---
Nitpick comments:
In `@e2e/react-start/basic/tests/prerendering.spec.ts`:
- Around line 7-11: The distDir is currently built from process.cwd(), which
couples the test to the invocation directory; change distDir to resolve relative
to the spec file by using fileURLToPath(import.meta.url) and dirname (e.g.,
import { fileURLToPath } from 'url' and import { dirname, join } from 'path')
and compute distDir via join(dirname(fileURLToPath(import.meta.url)), '..',
process.env.E2E_DIST_DIR ?? 'dist', 'client') so the variable distDir (used in
this spec) is robust regardless of the current working directory.
In `@e2e/react-start/basic/tests/utils/getE2EPortKey.ts`:
- Around line 1-5: The getE2EPortKey helper is duplicated across e2e suites;
extract getE2EPortKey (and its dependency getPackageName) into the shared
package `@tanstack/router-e2e-utils` and update callers to import from that
package instead of the local path; specifically, move the implementations for
getE2EPortKey() and getPackageName() into the shared module, export them, then
replace the local import in the file that currently imports
'./getPackageName.ts' with an import from '@tanstack/router-e2e-utils' and
ensure getE2EPortKey() continues to return process.env.E2E_PORT_KEY ??
getPackageName().
In `@e2e/solid-start/basic/playwright.config.ts`:
- Around line 13-21: Extract the repeated port-file cleanup into a shared helper
(e.g. export function cleanupPortFiles(e2ePortKey: string)) in the
`@tanstack/router-e2e-utils` module alongside
getTestServerPort/getDummyServerPort; the helper should perform the
TEST_WORKER_INDEX guard and call fs.rmSync on `port-${e2ePortKey}.txt`,
`port-${e2ePortKey}_start.txt`, and `port-${e2ePortKey}-external.txt` with {
force: true }. Then replace the duplicated block in playwright.config.ts (and
the matching blocks in vue-start/react-start) with a call to
cleanupPortFiles(e2ePortKey). Ensure the helper is exported and imported where
used.
In `@scripts/nx/playwright-plugin.md`:
- Around line 146-160: The cleanup loop that removes stale port files (the
fs.rmSync loop) is missing the `_start` variant; update the array of portFile
names used with e2ePortKey so it includes `port-${e2ePortKey}_start.txt`
alongside `port-${e2ePortKey}.txt` and `port-${e2ePortKey}-external.txt` (the
code using e2ePortKey and fs.rmSync should remove the `_start` file to match
getTestServerPort(`${e2ePortKey}_start`) behavior).
- Around line 60-73: Update the glob-like target notation to match actual
generated target names: replace occurrences of "build:e2e:*" and "test:e2e:*"
with "build:e2e--*" and "test:e2e--*" (or add parenthetical text "each inferred
build:e2e--… and test:e2e--… target") so the examples and prose match the real
targets (e.g. build:e2e--vite-ssr); adjust the sentence that lists injected env
vars (the block referencing MODE, TOOLCHAIN, E2E_TOOLCHAIN, E2E_DIST,
E2E_DIST_DIR, and E2E_PORT_KEY) to use the corrected target notation.
In `@scripts/nx/playwright-plugin.ts`:
- Around line 192-225: In validateModeMetadata, change the error message prefix
from "[Playwright Sharding Plugin]" to a more general "[Playwright Plugin]" for
all mode/toolchain validation errors thrown using modeMetadata and index (the
three throws that call isPlaywrightToolchain, isPlaywrightMode, and the missing
toolchain/mode check); update the messages that reference PLAYWRIGHT_TOOLCHAINS
and PLAYWRIGHT_MODES so the only change is the prefix text while leaving the
rest of each string intact.
- Around line 201-235: validateModeMetadata currently calls normalizeShardCount
only for its side effects and returns the original modeMetadata.shards, causing
callers (buildModeTargets) to re-run normalization; change validateModeMetadata
to capture and return the normalized shard count (call
normalizeShardCount(modeMetadata.shards, targetName) and use its result in the
returned PlaywrightModeMetadata.shards) and then update buildModeTargets to use
the returned normalized shards instead of calling normalizeShardCount again;
update any type annotations for PlaywrightModeMetadata.shards if needed so the
normalized value is propagated rather than recomputed.
- Around line 237-270: buildModeTargets silently overwrites duplicate
{toolchain, mode} entries when iterating modes; add a uniqueness guard (either
in validateModeMetadata or at start of the loop in buildModeTargets) that tracks
seen keys (e.g. derive a unique key from getModeTargetName(modeMetadata) or from
modeMetadata.toolchain+':' + modeMetadata.mode) and throws a clear error if a
duplicate is encountered so targets and targetGroup cannot be silently
overwritten; update validateModeMetadata or the loop to perform this check
before inserting into targets/targetGroup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24ef0334-5557-475f-ba18-7e0f67252fd2
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (63)
e2e/react-start/basic-prerender/.gitignoree2e/react-start/basic-prerender/package.jsone2e/react-start/basic-prerender/playwright.config.tse2e/react-start/basic-preview/.gitignoree2e/react-start/basic-preview/package.jsone2e/react-start/basic-preview/playwright.config.tse2e/react-start/basic-spa/.gitignoree2e/react-start/basic-spa/package.jsone2e/react-start/basic-spa/playwright.config.tse2e/react-start/basic-test-suite/.gitignoree2e/react-start/basic-test-suite/package.jsone2e/react-start/basic-test-suite/src/utils/getBasicAppRoot.tse2e/react-start/basic-test-suite/src/utils/isPrerender.tse2e/react-start/basic-test-suite/src/utils/isPreview.tse2e/react-start/basic-test-suite/src/utils/isSpaMode.tse2e/react-start/basic-test-suite/tsconfig.jsone2e/react-start/basic/.gitignoree2e/react-start/basic/package.jsone2e/react-start/basic/playwright.config.tse2e/react-start/basic/server.jse2e/react-start/basic/tests/client-only.spec.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/prerendering.spec.tse2e/react-start/basic/tests/raw-stream.spec.tse2e/react-start/basic/tests/redirect.spec.tse2e/react-start/basic/tests/root-scripts.spec.tse2e/react-start/basic/tests/script-duplication.spec.tse2e/react-start/basic/tests/search-params.spec.tse2e/react-start/basic/tests/setup/global.setup.tse2e/react-start/basic/tests/setup/global.teardown.tse2e/react-start/basic/tests/setup/waitForDummyServer.tse2e/react-start/basic/tests/special-characters.spec.tse2e/react-start/basic/tests/streaming.spec.tse2e/react-start/basic/tests/type-only-reexport.spec.tse2e/react-start/basic/tests/utils/getE2EPortKey.tse2e/react-start/basic/tests/utils/getPackageName.tse2e/react-start/basic/tsconfig.jsone2e/react-start/basic/vite.config.tse2e/solid-start/basic-test-suite/src/redirect.spec.tse2e/solid-start/basic-test-suite/src/setup/global.setup.tse2e/solid-start/basic-test-suite/src/setup/global.teardown.tse2e/solid-start/basic-test-suite/src/setup/waitForDummyServer.tse2e/solid-start/basic-test-suite/src/utils/getE2EPortKey.tse2e/solid-start/basic/.gitignoree2e/solid-start/basic/package.jsone2e/solid-start/basic/playwright.config.tse2e/solid-start/basic/server.jse2e/solid-start/basic/vite.config.tse2e/vue-start/basic-test-suite/src/redirect.spec.tse2e/vue-start/basic-test-suite/src/setup/global.setup.tse2e/vue-start/basic-test-suite/src/setup/global.teardown.tse2e/vue-start/basic-test-suite/src/setup/waitForDummyServer.tse2e/vue-start/basic-test-suite/src/utils/getE2EPortKey.tse2e/vue-start/basic/.gitignoree2e/vue-start/basic/package.jsone2e/vue-start/basic/playwright.config.tse2e/vue-start/basic/server.jse2e/vue-start/basic/vite.config.tspackage.jsonscripts/nx/playwright-plugin.mdscripts/nx/playwright-plugin.ts
💤 Files with no reviewable changes (16)
- e2e/react-start/basic-preview/.gitignore
- e2e/react-start/basic-test-suite/src/utils/isPrerender.ts
- e2e/react-start/basic-spa/.gitignore
- e2e/react-start/basic-prerender/.gitignore
- e2e/react-start/basic-test-suite/src/utils/isSpaMode.ts
- e2e/react-start/basic-test-suite/.gitignore
- e2e/react-start/basic-test-suite/src/utils/getBasicAppRoot.ts
- e2e/react-start/basic-test-suite/package.json
- e2e/react-start/basic-test-suite/src/utils/isPreview.ts
- e2e/react-start/basic-test-suite/tsconfig.json
- e2e/react-start/basic-preview/package.json
- e2e/react-start/basic-prerender/package.json
- e2e/react-start/basic-prerender/playwright.config.ts
- e2e/react-start/basic-spa/playwright.config.ts
- e2e/react-start/basic-preview/playwright.config.ts
- e2e/react-start/basic-spa/package.json
| "test:e2e": "rm -rf dist; rm -rf port*.txt; playwright test --project=chromium" | ||
| "test:e2e:startDummyServer": "node -e 'import(\"./tests/setup/global.setup.ts\").then(m => m.default())' & node -e 'import(\"./tests/setup/waitForDummyServer.ts\").then(m => m.default())'", | ||
| "test:e2e:stopDummyServer": "node -e 'import(\"./tests/setup/global.teardown.ts\").then(m => m.default())'", | ||
| "test:e2e:local": "playwright test --project=chromium" |
There was a problem hiding this comment.
test:e2e:local no longer performs a build or port-file cleanup.
The previous test:e2e script removed dist/port*.txt and implicitly relied on the webServer.command's pnpm build && pnpm start to produce fresh artifacts. The new flow assumes the mode-specific Nx build target ran beforehand (which the plugin wires in) and that playwright.config.ts handles port-file cleanup via fs.rmSync.
If a dev invokes pnpm test:e2e:local directly for a non-SSR mode (e.g. MODE=prerender E2E_DIST_DIR=dist-vite-prerender pnpm test:e2e:local) without first running the matching build:e2e--vite-prerender Nx target, Playwright will silently serve stale or missing artifacts. Consider a short README/AGENTS note — or a test:e2e:local:<mode> alias invoking the Nx build+test pair — so the non-Nx path stays ergonomic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/react-start/basic/package.json` at line 14, The test:e2e:local npm script
now only runs Playwright and can serve stale/missing artifacts if a matching Nx
build target (e.g., build:e2e--vite-prerender) wasn’t run first; update
package.json to add mode-aware convenience scripts or change test:e2e:local to
run the appropriate build+test sequence: detect MODE/E2E_DIST_DIR and invoke the
corresponding Nx build target (for example build:e2e--vite-prerender) before
running "playwright test --project=chromium", or add explicit
test:e2e:local:<mode> aliases that run "pnpm nx run <mode-specific-build> &&
pnpm test:e2e:local"; ensure any added script preserves the existing Playwright
cleanup behavior (playwright.config.ts uses fs.rmSync) so port-file/dist cleanup
still happens.
| const distDir = process.env.E2E_DIST_DIR || 'dist' | ||
| const distClientDir = path.resolve(distDir, 'client') | ||
| const distServerEntryPath = path.resolve(distDir, 'server', 'server.js') | ||
|
|
||
| export async function createStartServer() { | ||
| const server = (await import('./dist/server/server.js')).default | ||
| const server = (await import(distServerEntryPath)).default |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Node.js 20/22 support passing an absolute filesystem path (not a file:// URL) to dynamic import() on Linux?
💡 Result:
No, Node.js 20 and 22 do not support passing an absolute filesystem path (like "/home/user/file.js", without a file:// scheme) directly to dynamic import on Linux. The ESM loader requires specifiers to be valid URLs with supported schemes (file:, data:, node:). Absolute paths without a scheme trigger ERR_UNSUPPORTED_ESM_URL_SCHEME. Official docs state absolute specifiers must be like 'file:///opt/nodejs/config.js' [16,18,20]. The volume root can use '/' but this is a URL form, not raw path [16]. A test verifies dynamic import with absolute path strings (leading /) works on Unix [1], but requires conversion via pathToFileURL for raw paths [8,12]. Raw absolute paths fail, as seen in errors [8]. Use import(pathToFileURL('/path/to/file.js').href) instead.
Citations:
- 1: test: verify dynamic import call with absolute path strings nodejs/node#49275
- 2: https://stackoverflow.com/questions/77980517/unable-to-import-files-dynamically-in-nodejs-with-esm-modules
- 3: https://stackoverflow.com/questions/71432755/how-to-use-dynamic-import-from-a-dependency-in-node-js
- 4: https://nodejs.org/api/esm.html
- 5: https://nodejs.org/docs/latest/api/esm.html
- 6: https://nodejs.org/docs/latest-v20.x/api/esm.html
🏁 Script executed:
# Check if the absolute path import pattern exists in other e2e server files
rg "path\.resolve.*import\(" e2e/ -A 2 -B 2Repository: TanStack/router
Length of output: 41
🏁 Script executed:
# Find server.js files in e2e directories
fd "server\.js$" e2e/ --type fRepository: TanStack/router
Length of output: 207
🏁 Script executed:
# Check the solid-start server.js file
cat -n e2e/solid-start/basic/server.js | head -20Repository: TanStack/router
Length of output: 908
🏁 Script executed:
# Check the vue-start server.js file
cat -n e2e/vue-start/basic/server.js | head -20Repository: TanStack/router
Length of output: 908
Critical: absolute path passed to import() will fail on all platforms; also unify || to ?? for consistency.
The code passes a raw absolute filesystem path to import(), which Node.js 20/22 do not support on any platform (not just Windows). This will throw ERR_UNSUPPORTED_ESM_URL_SCHEME at runtime. The fix requires pathToFileURL() to convert the path to a valid file:// URL.
Additionally, line 12 uses || while e2e/react-start/basic/vite.config.ts and e2e/react-start/basic/playwright.config.ts use ??. Unifying on ?? prevents silent desynchronization if E2E_DIST_DIR is ever set to an empty string.
This affects e2e/react-start/basic/server.js, e2e/solid-start/basic/server.js, and e2e/vue-start/basic/server.js.
🔧 Required fix
+import { pathToFileURL } from 'node:url'
import { toNodeHandler } from 'srvx/node'
import path from 'node:path'
...
-const distDir = process.env.E2E_DIST_DIR || 'dist'
+const distDir = process.env.E2E_DIST_DIR ?? 'dist'
const distClientDir = path.resolve(distDir, 'client')
const distServerEntryPath = path.resolve(distDir, 'server', 'server.js')
export async function createStartServer() {
- const server = (await import(distServerEntryPath)).default
+ const server = (await import(pathToFileURL(distServerEntryPath).href)).default📝 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.
| const distDir = process.env.E2E_DIST_DIR || 'dist' | |
| const distClientDir = path.resolve(distDir, 'client') | |
| const distServerEntryPath = path.resolve(distDir, 'server', 'server.js') | |
| export async function createStartServer() { | |
| const server = (await import('./dist/server/server.js')).default | |
| const server = (await import(distServerEntryPath)).default | |
| import { pathToFileURL } from 'node:url' | |
| import { toNodeHandler } from 'srvx/node' | |
| import path from 'node:path' | |
| const distDir = process.env.E2E_DIST_DIR ?? 'dist' | |
| const distClientDir = path.resolve(distDir, 'client') | |
| const distServerEntryPath = path.resolve(distDir, 'server', 'server.js') | |
| export async function createStartServer() { | |
| const server = (await import(pathToFileURL(distServerEntryPath).href)).default |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/react-start/basic/server.js` around lines 12 - 17, Change the distDir
assignment to use nullish coalescing (distDir = process.env.E2E_DIST_DIR ??
'dist'), and convert the filesystem path before dynamic import: replace the raw
import(distServerEntryPath) in createStartServer with an import of a file:// URL
created via pathToFileURL(distServerEntryPath).toString() (ensure pathToFileURL
is imported from 'url'); apply the same pattern for
distClientDir/distServerEntryPath usage in createStartServer (and replicate
these changes in the sibling files for solid-start and vue-start).
| const commandByMode = | ||
| mode === 'preview' | ||
| ? `pnpm run test:e2e:startDummyServer && pnpm preview --outDir ${distDir} --port ${PORT}` | ||
| : `pnpm run test:e2e:startDummyServer && pnpm start` |
There was a problem hiding this comment.
preview branch skips build; depends on Nx build target running first.
pnpm preview --outDir ${distDir} --port ${PORT} only serves the already-built distDir. For Playwright invocations driven via the Nx plugin's generated test:e2e--vite-preview target (which depends on the matching build:e2e--vite-preview), this is correct. Direct local invocations (MODE=preview E2E_DIST_DIR=... pnpm test:e2e:local) will hit an empty dist. Worth documenting or guarding — e.g. prepending pnpm build && to the preview branch so the command is self-sufficient outside Nx.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/solid-start/basic/playwright.config.ts` around lines 27 - 30, The preview
branch of the command string (variable commandByMode) only serves distDir and
can run against an empty build when invoked locally; update commandByMode so the
preview branch prepends a build step (e.g., run the appropriate pnpm build
command before `pnpm preview --outDir ${distDir} --port ${PORT}`) so it is
self-sufficient outside Nx, and ensure any references to distDir/PORT remain
unchanged; alternatively document this requirement if you prefer not to change
commandByMode.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
e2e/vue-start/basic/tests/prerendering.spec.ts (1)
7-11: Consider anchoringdistDirto this file rather thanprocess.cwd().
vite.config.tsresolvesoutDirrelative to the project root (where the vite config lives), but this spec resolves it relative toprocess.cwd(). Those happen to agree when Playwright is invoked from the project directory (the usualpnpm test:e2e:local/ inferred Nx target flow), but the previousgetBasicAppRoot()was cwd-independent. A small tweak preserves that robustness:♻️ Proposed refactor
+import { fileURLToPath } from 'node:url' +import { dirname, resolve } from 'node:path' import { existsSync, readFileSync } from 'node:fs' import { join } from 'node:path' ... -const distDir = join( - process.cwd(), - process.env.E2E_DIST_DIR ?? 'dist', - 'client', -) +const projectRoot = resolve(dirname(fileURLToPath(import.meta.url)), '../..') +const distDir = join(projectRoot, process.env.E2E_DIST_DIR ?? 'dist', 'client')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/vue-start/basic/tests/prerendering.spec.ts` around lines 7 - 11, The distDir is anchored to process.cwd(), making the test fragile; change it to resolve relative to this spec file instead (use the test file's directory as the base) so it matches how vite.config.ts is resolved. Replace the use of process.cwd() when building distDir (the const distDir) with a path derived from this file (e.g., using fileURLToPath(import.meta.url)/path.dirname or the existing getBasicAppRoot() helper) so distDir is computed consistently with vite's outDir.scripts/nx/playwright-plugin.ts (1)
40-60: Nit:'playwright:sharded'tag is misleading for mode-based projects.Projects that declare
playwrightModeswithoutshards(or withshards: 1) are not sharded, yet still receive theplaywright:shardedtag. Consider a distinct tag such as'playwright:modes'(or adding it alongside) so downstream queries (nx show projects --with-tag) can target the two flows independently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/nx/playwright-plugin.ts` around lines 40 - 60, The current branch that handles playrightModes always tags the generated project with 'playwright:sharded' even when no sharding is used; update the return in the block that calls buildModeTargets(root, packageName, playwrightModes) so that it uses a distinct tag for mode-based projects (for example add or replace with 'playwright:modes') instead of 'playwright:sharded' — locate the object returned for [root] in this block and change the tags array accordingly so downstream queries can distinguish mode-only projects from true sharded projects.
🤖 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/solid-start/basic/tests/utils/getE2EPortKey.ts`:
- Around line 3-5: The getE2EPortKey function currently uses the nullish
coalescing operator so an empty string from process.env.E2E_PORT_KEY is treated
as a valid key; update getE2EPortKey to treat blank/whitespace values as unset
by checking the env var (e.g., trim and test length or use a boolean check) and
fall back to getPackageName() when the env var is undefined, null, or
empty/whitespace-only; locate the logic in getE2EPortKey and replace the
nullish-coalescing behavior with this explicit emptiness check.
In `@e2e/vue-start/basic/tests/utils/getE2EPortKey.ts`:
- Around line 3-5: The getE2EPortKey function currently uses the nullish
coalescing operator on process.env.E2E_PORT_KEY which preserves an empty string;
change getE2EPortKey so that an empty or whitespace-only E2E_PORT_KEY is treated
as unset and falls back to getPackageName(). In other words, check
process.env.E2E_PORT_KEY for truthiness/after trimming (or verify length > 0)
and only return it when non-empty; otherwise return getPackageName() to avoid
producing an empty port key.
In `@scripts/nx/playwright-plugin.ts`:
- Around line 148-163: validateModeMetadata currently validates shards by
coercing Number(modeMetadata.shards) but returns the original
modeMetadata.shards (possibly a string), causing downstream strict-equality
bugs; update the return to include shards: shardCount (the parsed integer)
instead of modeMetadata.shards so buildModeTargets and comparisons like
shardCount === 1 operate on a guaranteed number; ensure you still throw the same
error when Number.isInteger(shardCount) or shardCount < 1 fails and keep
references to targetName/modeMetadata for context in the error message.
- Around line 188-194: The modeEnv object currently exposes unused environment
keys; remove the keys TOOLCHAIN, E2E_TOOLCHAIN, and E2E_DIST from the modeEnv
literal so it only exports the required variables (keep MODE and E2E_DIST_DIR as
produced from modeMetadata and distDir). Locate the modeEnv declaration and
delete those three properties (they are the properties assigned from
modeMetadata.toolchain and distDir as E2E_DIST), leaving the rest of the
function and any other env keys untouched.
---
Nitpick comments:
In `@e2e/vue-start/basic/tests/prerendering.spec.ts`:
- Around line 7-11: The distDir is anchored to process.cwd(), making the test
fragile; change it to resolve relative to this spec file instead (use the test
file's directory as the base) so it matches how vite.config.ts is resolved.
Replace the use of process.cwd() when building distDir (the const distDir) with
a path derived from this file (e.g., using
fileURLToPath(import.meta.url)/path.dirname or the existing getBasicAppRoot()
helper) so distDir is computed consistently with vite's outDir.
In `@scripts/nx/playwright-plugin.ts`:
- Around line 40-60: The current branch that handles playrightModes always tags
the generated project with 'playwright:sharded' even when no sharding is used;
update the return in the block that calls buildModeTargets(root, packageName,
playwrightModes) so that it uses a distinct tag for mode-based projects (for
example add or replace with 'playwright:modes') instead of 'playwright:sharded'
— locate the object returned for [root] in this block and change the tags array
accordingly so downstream queries can distinguish mode-only projects from true
sharded projects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fb1b1a4-44c8-4c26-b98c-c2f3543f25a5
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (69)
e2e/react-start/basic/package.jsone2e/solid-start/basic-prerender/.gitignoree2e/solid-start/basic-prerender/package.jsone2e/solid-start/basic-prerender/playwright.config.tse2e/solid-start/basic-preview/.gitignoree2e/solid-start/basic-preview/package.jsone2e/solid-start/basic-preview/playwright.config.tse2e/solid-start/basic-spa/.gitignoree2e/solid-start/basic-spa/package.jsone2e/solid-start/basic-spa/playwright.config.tse2e/solid-start/basic-test-suite/.gitignoree2e/solid-start/basic-test-suite/package.jsone2e/solid-start/basic-test-suite/src/utils/getBasicAppRoot.tse2e/solid-start/basic-test-suite/src/utils/isPrerender.tse2e/solid-start/basic-test-suite/src/utils/isPreview.tse2e/solid-start/basic-test-suite/src/utils/isSpaMode.tse2e/solid-start/basic-test-suite/tsconfig.jsone2e/solid-start/basic/package.jsone2e/solid-start/basic/playwright.config.tse2e/solid-start/basic/tests/navigation.spec.tse2e/solid-start/basic/tests/not-found.spec.tse2e/solid-start/basic/tests/prerendering.spec.tse2e/solid-start/basic/tests/redirect.spec.tse2e/solid-start/basic/tests/script-duplication.spec.tse2e/solid-start/basic/tests/search-params.spec.tse2e/solid-start/basic/tests/setup/global.setup.tse2e/solid-start/basic/tests/setup/global.teardown.tse2e/solid-start/basic/tests/setup/waitForDummyServer.tse2e/solid-start/basic/tests/special-characters.spec.tse2e/solid-start/basic/tests/streaming.spec.tse2e/solid-start/basic/tests/transition.spec.tse2e/solid-start/basic/tests/utils/getE2EPortKey.tse2e/solid-start/basic/tests/utils/getPackageName.tse2e/solid-start/basic/tsconfig.jsone2e/vue-start/basic-prerender/.gitignoree2e/vue-start/basic-prerender/package.jsone2e/vue-start/basic-prerender/playwright.config.tse2e/vue-start/basic-preview/.gitignoree2e/vue-start/basic-preview/package.jsone2e/vue-start/basic-preview/playwright.config.tse2e/vue-start/basic-spa/.gitignoree2e/vue-start/basic-spa/package.jsone2e/vue-start/basic-spa/playwright.config.tse2e/vue-start/basic-test-suite/.gitignoree2e/vue-start/basic-test-suite/package.jsone2e/vue-start/basic-test-suite/src/utils/getBasicAppRoot.tse2e/vue-start/basic-test-suite/src/utils/isPrerender.tse2e/vue-start/basic-test-suite/src/utils/isPreview.tse2e/vue-start/basic-test-suite/src/utils/isSpaMode.tse2e/vue-start/basic-test-suite/tsconfig.jsone2e/vue-start/basic/package.jsone2e/vue-start/basic/playwright.config.tse2e/vue-start/basic/tests/navigation.spec.tse2e/vue-start/basic/tests/not-found.spec.tse2e/vue-start/basic/tests/prerendering.spec.tse2e/vue-start/basic/tests/redirect.spec.tse2e/vue-start/basic/tests/script-duplication.spec.tse2e/vue-start/basic/tests/search-params.spec.tse2e/vue-start/basic/tests/setup/global.setup.tse2e/vue-start/basic/tests/setup/global.teardown.tse2e/vue-start/basic/tests/setup/waitForDummyServer.tse2e/vue-start/basic/tests/special-characters.spec.tse2e/vue-start/basic/tests/streaming.spec.tse2e/vue-start/basic/tests/utils/getE2EPortKey.tse2e/vue-start/basic/tests/utils/getPackageName.tse2e/vue-start/basic/tsconfig.jsonpackage.jsonscripts/nx/playwright-plugin.mdscripts/nx/playwright-plugin.ts
💤 Files with no reviewable changes (32)
- e2e/solid-start/basic-spa/.gitignore
- e2e/vue-start/basic-test-suite/.gitignore
- e2e/vue-start/basic-prerender/.gitignore
- e2e/vue-start/basic-spa/.gitignore
- e2e/vue-start/basic-test-suite/src/utils/isSpaMode.ts
- e2e/vue-start/basic-preview/.gitignore
- e2e/solid-start/basic-test-suite/tsconfig.json
- e2e/solid-start/basic-preview/.gitignore
- e2e/solid-start/basic-prerender/.gitignore
- e2e/vue-start/basic-test-suite/src/utils/isPreview.ts
- e2e/vue-start/basic-test-suite/src/utils/isPrerender.ts
- e2e/solid-start/basic-test-suite/src/utils/isPreview.ts
- e2e/solid-start/basic-test-suite/src/utils/isSpaMode.ts
- e2e/solid-start/basic-test-suite/src/utils/isPrerender.ts
- e2e/solid-start/basic-preview/package.json
- e2e/solid-start/basic-test-suite/package.json
- e2e/solid-start/basic-spa/package.json
- e2e/solid-start/basic-preview/playwright.config.ts
- e2e/solid-start/basic-test-suite/src/utils/getBasicAppRoot.ts
- e2e/vue-start/basic-test-suite/tsconfig.json
- e2e/vue-start/basic-test-suite/src/utils/getBasicAppRoot.ts
- e2e/vue-start/basic-test-suite/package.json
- e2e/solid-start/basic-prerender/package.json
- e2e/vue-start/basic-preview/package.json
- e2e/vue-start/basic-spa/package.json
- e2e/solid-start/basic-prerender/playwright.config.ts
- e2e/vue-start/basic-preview/playwright.config.ts
- e2e/vue-start/basic-prerender/playwright.config.ts
- e2e/solid-start/basic-test-suite/.gitignore
- e2e/vue-start/basic-spa/playwright.config.ts
- e2e/vue-start/basic-prerender/package.json
- e2e/solid-start/basic-spa/playwright.config.ts
✅ Files skipped from review due to trivial changes (3)
- e2e/solid-start/basic/tsconfig.json
- e2e/vue-start/basic/tsconfig.json
- scripts/nx/playwright-plugin.md
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/solid-start/basic/playwright.config.ts
| export function getE2EPortKey() { | ||
| return process.env.E2E_PORT_KEY ?? getPackageName() | ||
| } |
There was a problem hiding this comment.
Treat blank E2E_PORT_KEY as unset.
?? only falls back for null/undefined, so E2E_PORT_KEY='' becomes the effective port key. That undermines mode-specific isolation by mapping to a shared empty-key port file.
Proposed fix
export function getE2EPortKey() {
- return process.env.E2E_PORT_KEY ?? getPackageName()
+ return process.env.E2E_PORT_KEY || getPackageName()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/solid-start/basic/tests/utils/getE2EPortKey.ts` around lines 3 - 5, The
getE2EPortKey function currently uses the nullish coalescing operator so an
empty string from process.env.E2E_PORT_KEY is treated as a valid key; update
getE2EPortKey to treat blank/whitespace values as unset by checking the env var
(e.g., trim and test length or use a boolean check) and fall back to
getPackageName() when the env var is undefined, null, or empty/whitespace-only;
locate the logic in getE2EPortKey and replace the nullish-coalescing behavior
with this explicit emptiness check.
| export function getE2EPortKey() { | ||
| return process.env.E2E_PORT_KEY ?? getPackageName() | ||
| } |
There was a problem hiding this comment.
Treat blank E2E_PORT_KEY as unset.
Using ?? preserves E2E_PORT_KEY='', which would make the port key empty and collapse port mapping to a shared port-.txt. Since this key is intended to isolate e2e modes, fall back when the env var is blank.
Proposed fix
export function getE2EPortKey() {
- return process.env.E2E_PORT_KEY ?? getPackageName()
+ return process.env.E2E_PORT_KEY || getPackageName()
}📝 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.
| export function getE2EPortKey() { | |
| return process.env.E2E_PORT_KEY ?? getPackageName() | |
| } | |
| export function getE2EPortKey() { | |
| return process.env.E2E_PORT_KEY || getPackageName() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/vue-start/basic/tests/utils/getE2EPortKey.ts` around lines 3 - 5, The
getE2EPortKey function currently uses the nullish coalescing operator on
process.env.E2E_PORT_KEY which preserves an empty string; change getE2EPortKey
so that an empty or whitespace-only E2E_PORT_KEY is treated as unset and falls
back to getPackageName(). In other words, check process.env.E2E_PORT_KEY for
truthiness/after trimming (or verify length > 0) and only return it when
non-empty; otherwise return getPackageName() to avoid producing an empty port
key.
| if (modeMetadata.shards !== undefined) { | ||
| const shardCount = Number(modeMetadata.shards) | ||
|
|
||
| if (!Number.isInteger(shardCount) || shardCount < 1) { | ||
| throw new Error( | ||
| `[Playwright Sharding Plugin] Invalid shard count for ${targetName}: ${modeMetadata.shards}. ` + | ||
| `Expected a positive integer.`, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| toolchain: modeMetadata.toolchain as PlaywrightToolchain, | ||
| mode: modeMetadata.mode as PlaywrightMode, | ||
| shards: modeMetadata.shards, | ||
| } |
There was a problem hiding this comment.
validateModeMetadata returns the unvalidated raw shards value.
The function coerces modeMetadata.shards via Number(...) for validation but then returns shards: modeMetadata.shards (the raw input) on line 162. If the JSON happens to carry a stringified number (e.g. "3"), validation passes but the string leaks into buildModeTargets, where shardCount === 1 on line 218 becomes a strict-equality mismatch and the value is then interpolated/compared via implicit coercion. Return the parsed integer so the rest of the function operates on a guaranteed number.
🛡️ Proposed fix
- if (modeMetadata.shards !== undefined) {
- const shardCount = Number(modeMetadata.shards)
-
- if (!Number.isInteger(shardCount) || shardCount < 1) {
- throw new Error(
- `[Playwright Sharding Plugin] Invalid shard count for ${targetName}: ${modeMetadata.shards}. ` +
- `Expected a positive integer.`,
- )
- }
- }
-
- return {
- toolchain: modeMetadata.toolchain as PlaywrightToolchain,
- mode: modeMetadata.mode as PlaywrightMode,
- shards: modeMetadata.shards,
- }
+ let shards: number | undefined
+ if (modeMetadata.shards !== undefined) {
+ const shardCount = Number(modeMetadata.shards)
+
+ if (!Number.isInteger(shardCount) || shardCount < 1) {
+ throw new Error(
+ `[Playwright Sharding Plugin] Invalid shard count for ${targetName}: ${modeMetadata.shards}. ` +
+ `Expected a positive integer.`,
+ )
+ }
+ shards = shardCount
+ }
+
+ return {
+ toolchain: modeMetadata.toolchain as PlaywrightToolchain,
+ mode: modeMetadata.mode as PlaywrightMode,
+ shards,
+ }📝 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.
| if (modeMetadata.shards !== undefined) { | |
| const shardCount = Number(modeMetadata.shards) | |
| if (!Number.isInteger(shardCount) || shardCount < 1) { | |
| throw new Error( | |
| `[Playwright Sharding Plugin] Invalid shard count for ${targetName}: ${modeMetadata.shards}. ` + | |
| `Expected a positive integer.`, | |
| ) | |
| } | |
| } | |
| return { | |
| toolchain: modeMetadata.toolchain as PlaywrightToolchain, | |
| mode: modeMetadata.mode as PlaywrightMode, | |
| shards: modeMetadata.shards, | |
| } | |
| let shards: number | undefined | |
| if (modeMetadata.shards !== undefined) { | |
| const shardCount = Number(modeMetadata.shards) | |
| if (!Number.isInteger(shardCount) || shardCount < 1) { | |
| throw new Error( | |
| `[Playwright Sharding Plugin] Invalid shard count for ${targetName}: ${modeMetadata.shards}. ` + | |
| `Expected a positive integer.`, | |
| ) | |
| } | |
| shards = shardCount | |
| } | |
| return { | |
| toolchain: modeMetadata.toolchain as PlaywrightToolchain, | |
| mode: modeMetadata.mode as PlaywrightMode, | |
| shards, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/nx/playwright-plugin.ts` around lines 148 - 163, validateModeMetadata
currently validates shards by coercing Number(modeMetadata.shards) but returns
the original modeMetadata.shards (possibly a string), causing downstream
strict-equality bugs; update the return to include shards: shardCount (the
parsed integer) instead of modeMetadata.shards so buildModeTargets and
comparisons like shardCount === 1 operate on a guaranteed number; ensure you
still throw the same error when Number.isInteger(shardCount) or shardCount < 1
fails and keep references to targetName/modeMetadata for context in the error
message.
| const modeEnv = { | ||
| MODE: modeMetadata.mode, | ||
| TOOLCHAIN: modeMetadata.toolchain, | ||
| E2E_TOOLCHAIN: modeMetadata.toolchain, | ||
| E2E_DIST: distDir, | ||
| E2E_DIST_DIR: distDir, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts -C2 "process\.env\.(MODE|TOOLCHAIN|E2E_TOOLCHAIN|E2E_DIST|E2E_DIST_DIR|E2E_MODE)\b"Repository: TanStack/router
Length of output: 13962
Remove unused env vars: TOOLCHAIN, E2E_TOOLCHAIN, E2E_DIST.
The grep search confirms that downstream code only reads MODE, E2E_DIST_DIR, and E2E_PORT_KEY. The variables TOOLCHAIN, E2E_TOOLCHAIN, and E2E_DIST are not referenced anywhere in the test suites and should be dropped to reduce noise.
(Note: MODE is intentional—it's the established contract with all Playwright configs and test setup files, which explicitly read process.env.MODE with fallbacks. While generically named, it's required here.)
🔧 Proposed cleanup
const modeEnv = {
MODE: modeMetadata.mode,
- TOOLCHAIN: modeMetadata.toolchain,
E2E_TOOLCHAIN: modeMetadata.toolchain,
- E2E_DIST: distDir,
E2E_DIST_DIR: distDir,
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/nx/playwright-plugin.ts` around lines 188 - 194, The modeEnv object
currently exposes unused environment keys; remove the keys TOOLCHAIN,
E2E_TOOLCHAIN, and E2E_DIST from the modeEnv literal so it only exports the
required variables (keep MODE and E2E_DIST_DIR as produced from modeMetadata and
distDir). Locate the modeEnv declaration and delete those three properties (they
are the properties assigned from modeMetadata.toolchain and distDir as
E2E_DIST), leaving the rest of the function and any other env keys untouched.
There was a problem hiding this comment.
Nx Cloud has identified a flaky task in your failed CI:
Since the failure was identified as flaky, the solution is to rerun CI. Because this branch comes from a fork, it is not possible for us to push directly, but you can rerun by pushing an empty commit:
git commit --allow-empty -m "chore: trigger rerun"
git push
🎓 Learn more about Self-Healing CI on nx.dev
1 participant
Summary
basice2e coverage into a single project by moving the shared suite intoe2e/react-start/basic/testsand removing the splitbasic-spa,basic-prerender,basic-preview, andbasic-test-suitewrappers.nx.metadata.playwrightModeswith bundler/mode validation, mode-specific inferred build+test targets, and mode-awareE2E_PORT_KEYenv wiring.e2e/react-start/basicto fresh mode-specific builds viavite build --outDir ${E2E_DIST_DIR:-dist}, and update Playwright/server setup to consumeE2E_DIST_DIRand local merged test setup paths.How the new setup works
nx.metadata.playwrightModes(for examplevite-ssr,vite-spa,vite-prerender,vite-preview).build:e2e--<mode>to build into a mode-specific output directory (E2E_DIST_DIR).test:e2e--<mode>to run Playwright with a uniqueE2E_PORT_KEYand matching env.e2e/react-start/basicPlaywright project is reused for every mode, and test behavior (server command, dist dir, etc.) is driven by env vars instead of duplicated projects.pnpm nx run tanstack-react-start-e2e-basic:test:e2e--vite-ssr), and Nx handles the corresponding build dependency automatically.Validation
pnpm exec eslint "scripts/nx/playwright-plugin.ts" "e2e/react-start/basic/playwright.config.ts" "e2e/react-start/basic/tests/**/*.ts"CI=1 NX_DAEMON=false pnpm nx show project tanstack-react-start-e2e-basic --jsonCI=1 NX_DAEMON=false pnpm nx run tanstack-react-start-e2e-basic:build:e2e--vite-prerender --outputStyle=stream --skipRemoteCacheMODE=ssr E2E_DIST_DIR=dist-vite-ssr E2E_PORT_KEY=tanstack-react-start-e2e-basic-vite-ssr pnpm --dir "e2e/react-start/basic" exec playwright test --project=chromium --listSummary by CodeRabbit
Bug Fixes
Refactor