fix(router-core): don't drop onRenderFinished listeners after stream fast path is reserved#7591
fix(router-core): don't drop onRenderFinished listeners after stream fast path is reserved#7591radosek wants to merge 1 commit into
Conversation
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughFixes an SSR streaming regression: onRenderFinished no longer drops listeners registered after the stream fast path is reserved; such listeners are retained and invoked (verified by a regression test and documented in a changeset). ChangesSSR Listener Execution Fix
Sequence Diagram(s)sequenceDiagram
participant SSRServer
participant RouterSsrQueryIntegration
participant QueryStream
RouterSsrQueryIntegration->>SSRServer: reserveStreamFastPath()
RouterSsrQueryIntegration->>SSRServer: onRenderFinished(register close callback)
SSRServer->>QueryStream: setRenderFinished() (later)
SSRServer->>RouterSsrQueryIntegration: invoke registered callback
RouterSsrQueryIntegration->>QueryStream: close()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
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 |
ae9d550 to
ad107d2
Compare
…th reserved When the stream fast path was already reserved, onRenderFinished silently dropped any listener registered afterwards. With @tanstack/react-router-ssr-query this meant the dehydration query stream was never closed, hanging the SSR response until the ~60s serialization timeout (TanStack#7529). The fast path still calls setRenderFinished() once the app stream ends, so the listener is now registered regardless and fires at the correct time instead of being discarded.
ad107d2 to
ea60ebe
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/router-core/src/ssr/ssr-server.ts`:
- Line 640: The single-line guard "if (cleanupStarted) return" must use braces
to comply with the TypeScript coding guideline; locate the conditional using the
cleanupStarted variable (in the SSR shutdown/cleanup routine where
cleanupStarted is checked) and replace the single-line form with a braced block
(e.g., if (cleanupStarted) { return; }) so the control statement always uses
curly braces.
🪄 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: 265c1e99-50a2-427c-b3ff-819c14e0feca
📒 Files selected for processing (3)
.changeset/cool-streams-close.mdpackages/router-core/src/ssr/ssr-server.tspackages/router-core/tests/ssr-server-cleanup.test.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/cool-streams-close.md
Fixes #7529
Problem
reserveStreamFastPath()(introduced in #7497) silently dropped anyonRenderFinishedlistener registered after the fast path was reserved:With
@tanstack/react-router-ssr-query, the integration registers anonRenderFinishedlistener (inrouter.options.dehydrate) that closes the dehydration query stream. When the fast path was reserved before that listener landed, the listener never ran, the query stream was never closed, andserializationFinishednever became true — so the SSR response hung until the ~60s serialization timeout. Browsers often paint early, but bots/curleat the full delay.router-core@1.171.7–1.171.9(@tanstack/react-router@1.170.x)router-core@1.171.6/@tanstack/react-router@1.169.0Fix
Register the listener regardless of the fast path. The fast path stream still calls
setRenderFinished()when the app stream ends (makeFastPathStream->finishSsrRendering()), so the listener fires at the correct time rather than being discarded. This keeps the original render-finished timing instead of invoking late listeners out of band.Test
Added a regression test in
ssr-server-cleanup.test.tsthat reserves the fast path, then registers anonRenderFinishedlistener and asserts it is not dropped (fires onsetRenderFinished). Verified it fails onmainand passes with the fix.router-coreunit suite: 1179 passed / 3 expected-fail (pre-existingit.failsdeepEqualdocs)router-ssr-query-core: passingSummary by CodeRabbit
Bug Fixes
Tests