test(solid-start): prerendering - sync #5475 to solid #5627
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR introduces end-to-end prerendering support for Solid Start. Changes include adding prerender mode detection utilities, new test scripts, updating Playwright and Vite configurations to support prerendering, adding a comprehensive prerendering test suite, and adjusting existing tests to account for prerender mode behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... |
❌ Failed | 8m 39s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 1s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-10-28 03:00:47 UTC
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/nitro-v2-vite-plugin
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-ssr-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-server
@tanstack/router-cli
@tanstack/router-core
@tanstack/router-devtools
@tanstack/router-devtools-core
@tanstack/router-generator
@tanstack/router-plugin
@tanstack/router-ssr-query-core
@tanstack/router-utils
@tanstack/router-vite-plugin
@tanstack/server-functions-plugin
@tanstack/solid-router
@tanstack/solid-router-devtools
@tanstack/solid-router-ssr-query
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-static-server-functions
@tanstack/start-storage-context
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
birkskyum
changed the title
![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 (2)
e2e/solid-start/basic/tests/prerendering.spec.ts (2)
10-31: Consider adding a dist directory existence check.The test assumes
dist/clientexists without verifying. If the build fails or the test runs from an unexpected location, the error messages could be confusing.Apply this diff to add an initial check:
test('should automatically discover and prerender static routes', () => { // Check that static routes were automatically discovered and prerendered const distDir = join(process.cwd(), 'dist', 'client') + expect(existsSync(distDir)).toBe(true) // These static routes should be automatically discovered and prerendered expect(existsSync(join(distDir, 'index.html'))).toBe(true)
34-52: Consider parameterizing the content verification tests.The two tests have identical structure and could be combined using test.each to reduce duplication and make it easier to add more content checks.
Example refactor:
test.describe('Static Files Verification', () => { test.each([ { route: 'posts', expectedContent: 'Select a post.' }, { route: 'users', expectedContent: 'Select a user.' }, ])('should contain prerendered content in $route.html', ({ route, expectedContent }) => { const distDir = join(process.cwd(), 'dist', 'client') const htmlPath = join(distDir, route, 'index.html') expect(existsSync(htmlPath)).toBe(true) const html = readFileSync(htmlPath, 'utf-8') expect(html).toContain(expectedContent) }) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e/solid-start/basic/package.json(1 hunks)e2e/solid-start/basic/playwright.config.ts(4 hunks)e2e/solid-start/basic/tests/prerendering.spec.ts(1 hunks)e2e/solid-start/basic/tests/search-params.spec.ts(4 hunks)e2e/solid-start/basic/tests/utils/isPrerender.ts(1 hunks)e2e/solid-start/basic/vite.config.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
e2e/solid-start/basic/tests/utils/isPrerender.tse2e/solid-start/basic/vite.config.tse2e/solid-start/basic/tests/search-params.spec.tse2e/solid-start/basic/playwright.config.tse2e/solid-start/basic/tests/prerendering.spec.ts
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/solid-start/basic/tests/utils/isPrerender.tse2e/solid-start/basic/package.jsone2e/solid-start/basic/vite.config.tse2e/solid-start/basic/tests/search-params.spec.tse2e/solid-start/basic/playwright.config.tse2e/solid-start/basic/tests/prerendering.spec.ts
**/package.json
📄 CodeRabbit inference engine (AGENTS.md)
Use workspace:* protocol for internal dependencies in package.json files
Files:
e2e/solid-start/basic/package.json
🧬 Code graph analysis (4)
e2e/solid-start/basic/vite.config.ts (1)
e2e/solid-start/basic/tests/utils/isPrerender.ts (1)
isPrerender(1-1)
e2e/solid-start/basic/tests/search-params.spec.ts (1)
e2e/solid-start/basic/tests/utils/isPrerender.ts (1)
isPrerender(1-1)
e2e/solid-start/basic/playwright.config.ts (2)
e2e/solid-start/basic/tests/utils/isSpaMode.ts (1)
isSpaMode(1-1)e2e/solid-start/basic/tests/utils/isPrerender.ts (1)
isPrerender(1-1)
e2e/solid-start/basic/tests/prerendering.spec.ts (1)
e2e/solid-start/basic/tests/utils/isPrerender.ts (1)
isPrerender(1-1)
🔇 Additional comments (11)
e2e/solid-start/basic/vite.config.ts (2)
6-6: LGTM!The import follows the established pattern of the
isSpaModeutility.
15-26: Clarify the filter logic concern — one example is invalid, and the suggested fix has issues.The substring matching concern is partially valid but contains errors:
- Valid: A path like
/redirect-mewould incorrectly match and be filtered out (false positive)- Invalid: A path
/not-found/something/elsewould NOT match/not-found/via-loader(that substring doesn't exist in the path)Additionally, the first suggested fix is incorrect:
.includes(page.path)checks if the array contains the path as an element (exact matching), which is not the intended behavior.The
.startsWith()approach would be appropriate if you want prefix-based exclusion of all nested routes under/redirectand/not-found. However, you should verify whether the current substring-matching behavior is intentional given that the codebase has routes like/redirect/$target/via-beforeLoadthat would match the/redirectpattern regardless.e2e/solid-start/basic/tests/utils/isPrerender.ts (1)
1-1: LGTM!Clean utility following the established pattern of
isSpaMode.e2e/solid-start/basic/playwright.config.ts (3)
7-8: LGTM!The imports are correct, and the JSON import assertion syntax is appropriate.
28-28: LGTM!The console logging helps with debugging test execution modes.
46-46: LGTM!Using the
getCommand()helper improves code clarity.e2e/solid-start/basic/tests/search-params.spec.ts (2)
31-33: LGTM!The updated redirect logic correctly accounts for prerender mode. Prerendered pages are static, so server-side redirects don't occur at request time—they're handled during the build.
Also applies to: 56-58
69-69: LGTM!URL normalization by removing the unnecessary trailing slash before the query parameters improves consistency.
e2e/solid-start/basic/tests/prerendering.spec.ts (1)
1-8: LGTM!The imports are appropriate for filesystem-based prerender verification, and the conditional skip ensures tests only run in the correct mode.
e2e/solid-start/basic/package.json (2)
11-11: LGTM!The build script follows the established pattern and correctly sets the MODE environment variable.
18-19: LGTM!The test scripts properly integrate prerender mode into the e2e test workflow, and the sequential chaining with
&&ensures that failures in any mode stop the test run.
| const prerenderModeCommand = `pnpm run test:e2e:startDummyServer && pnpm build:prerender && pnpm run test:e2e:stopDummyServer && pnpm start` | ||
|
|
||
| const getCommand = () => { | ||
| if (isSpaMode) return spaModeCommand | ||
| if (isPrerender) return prerenderModeCommand | ||
| return ssrModeCommand | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Review the dummy server lifecycle management for potential issues.
The prerenderModeCommand chains commands with &&, which could lead to:
- Resource leak: If
build:prerenderfails,stopDummyServerwon't execute, leaving the dummy server running. - Race condition: There's no synchronization between
startDummyServerandbuild:prerender. If the dummy server takes time to initialize, the build might start before the server is ready.
The getCommand() helper refactoring is clean and improves readability.
Consider adding error handling to ensure cleanup:
-const prerenderModeCommand = `pnpm run test:e2e:startDummyServer && pnpm build:prerender && pnpm run test:e2e:stopDummyServer && pnpm start`
+const prerenderModeCommand = `pnpm run test:e2e:startDummyServer && (pnpm build:prerender; pnpm run test:e2e:stopDummyServer) && pnpm start`Or verify that the dummy server scripts include their own timeout/retry logic to handle startup delays.
🏁 Script executed:
#!/bin/bash
# View the playwright config file to see the current state
cat e2e/solid-start/basic/playwright.config.tsLength of output: 1965
🏁 Script executed:
#!/bin/bash
# Search for dummy server script definitions in package.json or other config files
fd -t f "package.json" | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && grep -A 2 -B 2 "startDummyServer\|stopDummyServer" {} 2>/dev/null || true'Length of output: 254
🏁 Script executed:
#!/bin/bash
# Check for any scripts directory or test setup files that might define the dummy server
fd -t f "(startDummyServer|stopDummyServer|dummy)" e2e/Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for any error handling or cleanup logic in e2e test setup
rg -n "trap\|catch\|error\|cleanup" e2e/solid-start/basic/ --type ts --type jsLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for dummy server script definitions in root package.json
cat package.json | grep -A 100 '"scripts"' | head -50Length of output: 2485
🏁 Script executed:
#!/bin/bash
# Search for these specific scripts across the entire codebase
rg -n "startDummyServer|stopDummyServer" --type json --type ts --type js -B 1 -A 1Length of output: 1672
🏁 Script executed:
#!/bin/bash
# Check if there's a workspace configuration that might define these scripts
fd -t f "pnpm-workspace.yaml|turbo.json|package.json" -x grep -l "startDummyServer\|stopDummyServer" {} \;Length of output: 73
🏁 Script executed:
#!/bin/bash
# View the global setup and teardown files to understand their robustness
cat e2e/solid-start/basic/tests/setup/global.setup.tsLength of output: 220
🏁 Script executed:
#!/bin/bash
# View teardown file
cat e2e/solid-start/basic/tests/setup/global.teardown.tsLength of output: 221
🏁 Script executed:
#!/bin/bash
# Check if there are any existing error handling patterns in the e2e config or setup
grep -r "trap\|error\|catch\|finally" e2e/solid-start/basic/tests/setup/ --include="*.ts" -nLength of output: 0
Address dummy server lifecycle management issues in prerender command.
The resource leak and race condition concerns are confirmed valid:
- Resource leak: If
build:prerenderfails,stopDummyServernever executes due to&&chaining, leaving the background dummy server process orphaned. - Race condition:
startDummyServerruns backgrounded (&), allowingbuild:prerenderto start immediately without explicit synchronization. The server may still be initializing when the build begins.
The suggested fix using subshell syntax (pnpm build:prerender; pnpm run test:e2e:stopDummyServer) does ensure cleanup runs, but it changes error propagation—if cleanup fails, the subsequent pnpm start also fails.
Verify and apply one of these approaches:
- Option A (Cleanup priority): Use the suggested subshell approach if cleanup failures should halt execution.
- Option B (Explicit trap): Use shell trap to guarantee cleanup:
trap 'pnpm run test:e2e:stopDummyServer' EXITbefore the build. - Option C (Explicit wait): Add explicit wait logic after
startDummyServerto ensure the dummy server is ready before proceeding.
Same issue exists in e2e/react-start/basic/playwright.config.ts line 20.
🤖 Prompt for AI Agents
In e2e/solid-start/basic/playwright.config.ts around lines 20 to 26, the
prerender command uses chained commands that can leak the background dummy
server and race with the build; replace the one-liner with a safe sequence that
starts the dummy server, waits for it to be ready, runs build:prerender, and
always stops the dummy server on exit—either (A) wrap build and cleanup in a
subshell so cleanup runs and failures propagate, (B) set a shell trap to run
pnpm run test:e2e:stopDummyServer on EXIT before starting the server, or (C) add
an explicit readiness wait after starting the server (e.g., polling a health
endpoint) then run build and finally stop the server; apply the same change to
e2e/react-start/basic/playwright.config.ts at line 20.
| import { isSpaMode } from 'tests/utils/isSpaMode' | ||
| import { isPrerender } from './utils/isPrerender' |
There was a problem hiding this comment.
Inconsistent import paths for utilities.
Line 3 uses tests/utils/isSpaMode (absolute/tsconfig path) while line 4 uses ./utils/isPrerender (relative path). For consistency, both utilities should use the same import pattern.
Apply this diff to make imports consistent:
-import { isSpaMode } from 'tests/utils/isSpaMode'
-import { isPrerender } from './utils/isPrerender'
+import { isSpaMode } from './utils/isSpaMode'
+import { isPrerender } from './utils/isPrerender'Or if tsconfig paths are preferred:
-import { isSpaMode } from 'tests/utils/isSpaMode'
-import { isPrerender } from './utils/isPrerender'
+import { isSpaMode } from 'tests/utils/isSpaMode'
+import { isPrerender } from 'tests/utils/isPrerender'📝 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.
| import { isSpaMode } from 'tests/utils/isSpaMode' | |
| import { isPrerender } from './utils/isPrerender' | |
| import { isSpaMode } from 'tests/utils/isSpaMode' | |
| import { isPrerender } from 'tests/utils/isPrerender' |
🤖 Prompt for AI Agents
In e2e/solid-start/basic/tests/search-params.spec.ts around lines 3 to 4, the
two utility imports use inconsistent paths (one using tsconfig/absolute
'tests/utils/isSpaMode' and the other a relative './utils/isPrerender'); make
them consistent by changing the import so both use the same pattern—either
update line 3 to a relative import './utils/isSpaMode' or update line 4 to the
tsconfig path 'tests/utils/isPrerender' depending on the project convention
(prefer tsconfig paths if that is the established pattern), and run a quick
compile to ensure the chosen path resolves.
7b61b60 to
7ea0729
Compare
birkskyum
force-pushed
the
sync-#5475-to-solid
branch
from
7ea0729 to
e87340c
Compare
birkskyum
force-pushed
the
sync-#5475-to-solid
branch
4 times, most recently
from
67fb5fd to
0f51b20
Compare
birkskyum
force-pushed
the
sync-#5475-to-solid
branch
from
0f51b20 to
7cc1c22
Compare
Blocked by
Related to
Summary by CodeRabbit
New Features
Tests