fix: preserve solid deferred stream order#7515
Open
brenelz wants to merge 3 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR enhances SSR streaming to detect and properly handle the ChangesIndependent Deferred Boundary Streaming
Possibly Related PRs
Suggested Labels
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
|
View your CI Pipeline Execution ↗ for commit 4955201
☁️ Nx Cloud last updated this comment at |
Contributor
🚀 Changeset Version PreviewNo changeset entries found. Merging this PR will not cause a version bump for any packages. |
Contributor
Bundle Size Benchmarks
Current gzip tracks all emitted client JS chunks. Initial gzip tracks only the entry/import graph. Trend sparkline is historical current gzip ending with this PR measurement; lower is better. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/solid-start/basic/tests/streaming.spec.ts`:
- Around line 31-45: The SSR-streaming specific assertions in the test 'deferred
route streams boundaries independently' should be skipped when running in SPA
mode: wrap the block of expectations that assert commit-time streamed HTML (the
checks for "Loading person...", "Loading stuff...", and the deferred element
contents like page.getByTestId('deferred-person') and
page.getByTestId('deferred-stuff')) in a guard that only runs when not in SPA
mode (e.g., if (!isSpaMode()) { ... }) or conditionally call test.skip when
isSpaMode() is true; reuse or add a project helper named isSpaMode() (or the
existing SPA-mode detection utility) to detect SPA runs and ensure the streaming
assertions are bypassed in SPA mode.
In `@packages/router-core/src/ssr/transformStreamWithRouter.ts`:
- Around line 813-836: The HTML end-tag detection fails when the tail is split
across chunks (e.g., previous leftover ends with "</ht" and current chunk begins
"ml>") because the code only calls findHtmlEndTagEnd on chunkString; change the
logic in the pending-tail branch to search the concatenation of the buffered
tail/leftover plus the current chunk (use the same findHtmlEndTagEnd on leftover
+ chunkString), adjust appendTail and slicing indices to account for the
combined string (so appendTail receives only the part up to htmlEndTagEnd from
the combined string, and afterClosingTags is the remainder from the chunk
portion), and ensure leftover is cleared/updated correctly so
flushPendingRouterHtml, noteBarrierMarker and subsequent writeChunk flows behave
the same as before.
🪄 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: c41419b5-f19b-4a40-b612-6a6cde9ebd9d
📒 Files selected for processing (4)
e2e/react-start/basic/tests/streaming.spec.tse2e/solid-start/basic/tests/streaming.spec.tspackages/router-core/src/ssr/transformStreamWithRouter.tspackages/solid-router/src/awaited.tsx
Comment on lines
+31
to
+45
| test('deferred route streams boundaries independently', async ({ page }) => { | ||
| await page.goto('/deferred', { waitUntil: 'commit' }) | ||
|
|
||
| await expect(page.getByTestId('regular-person')).toContainText('John Doe') | ||
| await expect(page.getByText('Loading person...')).toBeVisible() | ||
| await expect(page.getByText('Loading stuff...')).toBeVisible() | ||
|
|
||
| await expect(page.getByTestId('deferred-person')).toContainText( | ||
| 'Tanner Linsley', | ||
| ) | ||
| await expect(page.getByText('Loading stuff...')).toBeVisible() | ||
| await expect(page.getByTestId('deferred-stuff')).toContainText( | ||
| 'Hello deferred!', | ||
| ) | ||
| }) |
Contributor
There was a problem hiding this comment.
Skip this SSR-streaming assertion in SPA mode.
This test exercises commit-time streamed HTML behavior, so it has the same SPA-mode constraint as the existing SSR fallback test later in this file. Without the guard, the Solid suite can fail in SPA mode even though the app is behaving correctly.
💡 Suggested fix
test('deferred route streams boundaries independently', async ({ page }) => {
+ test.skip(isSpaMode, 'SPA mode does not render streamed SSR HTML')
await page.goto('/deferred', { waitUntil: 'commit' })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/solid-start/basic/tests/streaming.spec.ts` around lines 31 - 45, The
SSR-streaming specific assertions in the test 'deferred route streams boundaries
independently' should be skipped when running in SPA mode: wrap the block of
expectations that assert commit-time streamed HTML (the checks for "Loading
person...", "Loading stuff...", and the deferred element contents like
page.getByTestId('deferred-person') and page.getByTestId('deferred-stuff')) in a
guard that only runs when not in SPA mode (e.g., if (!isSpaMode()) { ... }) or
conditionally call test.skip when isSpaMode() is true; reuse or add a project
helper named isSpaMode() (or the existing SPA-mode detection utility) to detect
SPA runs and ensure the streaming assertions are bypassed in SPA mode.
Comment on lines
+813
to
+836
| if (!pendingTailComplete) { | ||
| const htmlEndTagEnd = findHtmlEndTagEnd(chunkString, 0) | ||
| if (htmlEndTagEnd === -1) { | ||
| appendTail(chunkString) | ||
| leftover = '' | ||
| continue | ||
| } | ||
|
|
||
| appendTail(chunkString.slice(0, htmlEndTagEnd)) | ||
| pendingTailComplete = true | ||
|
|
||
| const afterClosingTags = chunkString.slice(htmlEndTagEnd) | ||
| flushPendingRouterHtml() | ||
| if (afterClosingTags) { | ||
| writeChunk(afterClosingTags) | ||
| if (cleanedUp || isDone()) return | ||
| noteBarrierMarker(afterClosingTags) | ||
| liftBarrierAfterBoundary() | ||
| if (cleanedUp || isDone()) return | ||
| flushPendingRouterHtml() | ||
| } | ||
|
|
||
| leftover = '' | ||
| continue |
Contributor
There was a problem hiding this comment.
Handle </html> split across chunk boundaries.
Once HoldingTail starts, this scan only looks at the current chunkString. If the previous tail ended with something like </ht and the next chunk starts with ml>, the closing tag is never detected, so pendingTailComplete stays false and the tail keeps buffering until EOF.
💡 Suggested fix
if (state >= MergeState.HoldingTail) {
if (!pendingTailComplete) {
- const htmlEndTagEnd = findHtmlEndTagEnd(chunkString, 0)
+ const overlapChars = Math.min(6, pendingTail.length)
+ const overlap = pendingTail.slice(-overlapChars) + chunkString
+ const htmlEndTagEnd = findHtmlEndTagEnd(overlap, 0)
if (htmlEndTagEnd === -1) {
appendTail(chunkString)
leftover = ''
continue
}
- appendTail(chunkString.slice(0, htmlEndTagEnd))
+ const splitIndex = Math.max(0, htmlEndTagEnd - overlapChars)
+ appendTail(chunkString.slice(0, splitIndex))
pendingTailComplete = true
- const afterClosingTags = chunkString.slice(htmlEndTagEnd)
+ const afterClosingTags = chunkString.slice(splitIndex)
flushPendingRouterHtml()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/router-core/src/ssr/transformStreamWithRouter.ts` around lines 813 -
836, The HTML end-tag detection fails when the tail is split across chunks
(e.g., previous leftover ends with "</ht" and current chunk begins "ml>")
because the code only calls findHtmlEndTagEnd on chunkString; change the logic
in the pending-tail branch to search the concatenation of the buffered
tail/leftover plus the current chunk (use the same findHtmlEndTagEnd on leftover
+ chunkString), adjust appendTail and slicing indices to account for the
combined string (so appendTail receives only the part up to htmlEndTagEnd from
the combined string, and afterClosingTags is the remainder from the chunk
portion), and ensure leftover is cleared/updated correctly so
flushPendingRouterHtml, noteBarrierMarker and subsequent writeChunk flows behave
the same as before.
Merging this PR will not alter performance
Comparing 
Footnotes
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow up to this #7512
Summary by CodeRabbit
Tests
Improvements