fix(router-core): flush buffered scripts before closing SSR stream#7352
Open
roduyemi wants to merge 2 commits intoTanStack:mainfrom
Open
fix(router-core): flush buffered scripts before closing SSR stream#7352roduyemi wants to merge 2 commits intoTanStack:mainfrom
roduyemi wants to merge 2 commits intoTanStack:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR fixes missing scripts during SSR streaming by adding a public ChangesSSR Script Injection and Barrier Deferral
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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 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 |
roduyemi
changed the title
Contributor
|
View your CI Pipeline Execution ↗ for commit 1e2ff8d
☁️ Nx Cloud last updated this comment at |
roduyemi
changed the title
Contributor
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes 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.
🧹 Nitpick comments (3)
packages/router-core/tests/script-buffer.test.ts (1)
46-48: 💤 Low valueOptional: replace fixed-timeout wait with
onSerializationFinishedsubscription for determinism.The 50ms
setTimeoutis generous but can become flaky on busy CI runners. Subscribing to the existingonSerializationFinishedevent (or awaiting a promise that resolves on it) makes the assertion deterministic and faster. The same applies to lines 81–82 in the second test.Proposed change
- // Wait for deferred promise + crossSerializeStream.onDone - await new Promise((r) => setTimeout(r, 50)) - expect(router.serverSsr!.isSerializationFinished()).toBe(true) + // Wait until serialization completes deterministically + await new Promise<void>((resolve) => { + const unsub = router.subscribe('onSerializationFinished', () => { + unsub() + resolve() + }) + }) + expect(router.serverSsr!.isSerializationFinished()).toBe(true)🤖 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/tests/script-buffer.test.ts` around lines 46 - 48, Replace the fixed 50ms timeout with a deterministic wait for the serialization completion event: subscribe to router.serverSsr!.onSerializationFinished (or await a promise that resolves when that event fires) before asserting router.serverSsr!.isSerializationFinished(); do the same replacement for the other occurrence in the second test (lines around the second timeout) so tests no longer rely on setTimeout and become deterministic and faster.packages/router-core/src/ssr/ssr-server.ts (1)
112-144: 💤 Low valueOptional: collapse
flush()andinjectBufferedScripts()to remove duplication.The two methods perform nearly identical work —
takeAll()+injectScript(). Their only meaningful differences are early-return predicates. Consolidating into a single private helper (with aforceorrespectBarrierflag) would reduce drift risk.Sketch
private inject(respectBarrier: boolean) { if (this._cleanedUp) return if (respectBarrier && !this._scriptBarrierLifted) return if (this._queue.length === 0) return const scriptsToInject = this.takeAll() if (scriptsToInject && this.router?.serverSsr) { this.router.serverSsr.injectScript(scriptsToInject) } } flush() { this.inject(true) } injectBufferedScripts() { this.inject(false) }🤖 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/ssr-server.ts` around lines 112 - 144, The flush() and injectBufferedScripts() methods duplicate logic (calling takeAll() and router.serverSsr.injectScript) with only different early-return predicates; refactor by creating a private helper (e.g., inject(respectBarrier: boolean)) that centralizes the checks: return if this._cleanedUp, if respectBarrier && !this._scriptBarrierLifted, and if this._queue.length === 0, then call takeAll() and, if result and this.router?.serverSsr, call this.router.serverSsr.injectScript(scriptsToInject); replace flush() with this.inject(true) and injectBufferedScripts() with this.inject(false) to remove duplication while keeping semantics.packages/router-core/src/ssr/transformStreamWithRouter.ts (1)
437-451: 💤 Low valueDeferred
liftScriptBarrier()ordering is correct, with a minor redundancy.Lifting the barrier after
flushPendingRouterHtml()is right: it guarantees pending router HTML is emitted before any scripts queued post-lift get injected. Note that once the localstreamBarrierLiftedbecomestrue, it staystrue, so every subsequent closing-tag boundary will re-callliftScriptBarrier(). The call is idempotent (guarded by_scriptBarrierLifted), so this is harmless — just a tiny bit of repeated work. Optional: short-circuit locally after the first successful lift if you want to avoid the extra calls.🤖 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 437 - 451, The code repeatedly calls router.serverSsr?.liftScriptBarrier() whenever streamBarrierLifted is true even though the lift is idempotent; short-circuit after the first lift by checking and setting a local guard so subsequent branches skip the call. Modify the block that currently checks streamBarrierLifted (near findLastClosingTagEnd logic in transformStreamWithRouter) to only invoke router.serverSsr?.liftScriptBarrier() once: e.g., add or flip a local boolean (using the existing streamBarrierLifted or a new local like localBarrierLifted) to ensure liftScriptBarrier() and its setter run only on the first pass, keeping flushPendingRouterHtml(), safeEnqueue(), and findLastClosingTagEnd usage unchanged.
🤖 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.
Nitpick comments:
In `@packages/router-core/src/ssr/ssr-server.ts`:
- Around line 112-144: The flush() and injectBufferedScripts() methods duplicate
logic (calling takeAll() and router.serverSsr.injectScript) with only different
early-return predicates; refactor by creating a private helper (e.g.,
inject(respectBarrier: boolean)) that centralizes the checks: return if
this._cleanedUp, if respectBarrier && !this._scriptBarrierLifted, and if
this._queue.length === 0, then call takeAll() and, if result and
this.router?.serverSsr, call
this.router.serverSsr.injectScript(scriptsToInject); replace flush() with
this.inject(true) and injectBufferedScripts() with this.inject(false) to remove
duplication while keeping semantics.
In `@packages/router-core/src/ssr/transformStreamWithRouter.ts`:
- Around line 437-451: The code repeatedly calls
router.serverSsr?.liftScriptBarrier() whenever streamBarrierLifted is true even
though the lift is idempotent; short-circuit after the first lift by checking
and setting a local guard so subsequent branches skip the call. Modify the block
that currently checks streamBarrierLifted (near findLastClosingTagEnd logic in
transformStreamWithRouter) to only invoke router.serverSsr?.liftScriptBarrier()
once: e.g., add or flip a local boolean (using the existing streamBarrierLifted
or a new local like localBarrierLifted) to ensure liftScriptBarrier() and its
setter run only on the first pass, keeping flushPendingRouterHtml(),
safeEnqueue(), and findLastClosingTagEnd usage unchanged.
In `@packages/router-core/tests/script-buffer.test.ts`:
- Around line 46-48: Replace the fixed 50ms timeout with a deterministic wait
for the serialization completion event: subscribe to
router.serverSsr!.onSerializationFinished (or await a promise that resolves when
that event fires) before asserting router.serverSsr!.isSerializationFinished();
do the same replacement for the other occurrence in the second test (lines
around the second timeout) so tests no longer rely on setTimeout and become
deterministic and faster.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 511bcafb-71f1-4b86-8bcd-d9b61cdd9638
📒 Files selected for processing (3)
packages/router-core/src/ssr/ssr-server.tspackages/router-core/src/ssr/transformStreamWithRouter.tspackages/router-core/tests/script-buffer.test.ts
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.
Streaming scripts (
$_TSR.e()and deferred data resolvers) are silently dropped when serialisation finishes before the HTML stream is processed.Fixes #7349
Two root causes:
ScriptBuffer.enqueue()andliftBarrier()defer injection viaqueueMicrotaskwhich meanscleanup()runs synchronously before the microtask firestransformStreamWithRouternever callstakeBufferedHtml()aftersetRenderFinished()Changes
ScriptBuffer.enqueue()andliftBarrier()synchronousflushScripts()toServerSsrand calls it intryFinish()setRenderFinished()liftScriptBarrier()afterflushPendingRouterHtml()to ensure correct orderingSummary by CodeRabbit
New Features
flushScripts()method to enable manual flushing of queued server-side rendering scripts.Improvements