Skip to content

Conversation

@schiller-manuel
Copy link
Contributor

@schiller-manuel schiller-manuel commented Nov 25, 2025

Summary by CodeRabbit

  • Bug Fixes
    • Improved cleanup and resource management in server-side rendering stream handling.
    • Enhanced query cache subscription handling to ensure proper cleanup on render completion.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 25, 2025

Walkthrough

SSR stream handling is refactored to improve listener lifecycle management. Injected HTML listener setup replaces callback-based patterns with a dedicated handler function; cleanup operations add guards to prevent undefined references. Query cache subscription cleanup is properly initialized and invoked at render completion.

Changes

Cohort / File(s) Summary
SSR Stream Processing
packages/router-core/src/ssr/transformStreamWithRouter.ts
Reworked injected HTML listener setup from callback-based inner function to dedicated handleInjectedHtml() function. Added guarded cleanup for stopListeningToInjectedHtml. Processing already-injected HTML centralized into single handler invocation. Finalization logic deferred to ensure listener cleanup in all paths and injectedHtmlDonePromise resolves after in-flight promises complete.
Query Cache Cleanup
packages/router-ssr-query-core/src/index.ts
Introduced local unsubscribe variable to store query cache subscription cleanup function. Implemented unsubscribe invocation and stream closure at render completion, replacing direct subscription assignment.

Sequence Diagram

sequenceDiagram
    participant Router as Router SSR
    participant Listener as Listener
    participant Handler as handleInjectedHtml
    participant Buffer as routerStreamBuffer

    rect rgb(200, 220, 240)
    Note over Router,Buffer: New Flow
    Router->>Listener: setup listener to pass injected HTML
    Router->>Handler: initial call to process injectedHtml array
    loop for each promise
        Handler->>Handler: increment processingCount
        Handler->>Buffer: collect emitted HTML
    end
    Handler->>Handler: clear injectedHtml array
    
    rect rgb(220, 240, 200)
    Note over Handler,Buffer: Finalization (on render complete)
    end
    Handler->>Handler: wait for in-flight promises
    Handler->>Listener: cleanup listener (guarded)
    Handler->>Router: resolve injectedHtmlDonePromise
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Focus areas:
    • transformStreamWithRouter.ts: Complex refactoring of handleInjectedHtml() function including processing logic, state management with processingCount, and timing-sensitive finalization paths that defer cleanup
    • Promise sequencing and async flow in finalization to ensure injectedHtmlDonePromise resolves after all in-flight HTML promises complete
    • Interaction between listener setup, per-promise handlers, and deferred cleanup operations

Possibly related PRs

  • fix: script streaming #5895: Modifies the same transformStreamWithRouter.ts file to add TSR script barrier logic and decoding/write changes in the SSR stream finalization flow
  • fix: memory leaks #5896: Refactors the same router-core SSR stream listener lifecycle and teardown mechanisms for stopListeningToInjectedHtml cleanup handling

Suggested labels

package: router-core

Poem

🐰 A listener awakens, then bids goodbye,
With guards and cleanup, no more to cry,
HTML promises processed with grace,
Stream cleanup finalized—just the right place!
Query subscriptions now properly freed,
SSR rendering succeeds!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check ❓ Inconclusive The title 'fix: cleanup references' is generic and vague, lacking specificity about which cleanup references are being addressed or which components are affected. Consider a more descriptive title that specifies the cleanup being performed, such as 'fix: ensure proper cleanup of injected HTML and query cache listeners' or similar.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch cleanup-refs

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e41cdf2 and 9123be7.

📒 Files selected for processing (2)
  • packages/router-core/src/ssr/transformStreamWithRouter.ts (3 hunks)
  • packages/router-ssr-query-core/src/index.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use TypeScript strict mode with extensive type safety throughout the codebase

Files:

  • packages/router-ssr-query-core/src/index.ts
  • packages/router-core/src/ssr/transformStreamWithRouter.ts
🧠 Learnings (2)
📚 Learning: 2025-11-25T00:18:21.258Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T00:18:21.258Z
Learning: Applies to packages/solid-router/**/*.{ts,tsx} : Solid Router components and primitives should use the tanstack/solid-router package

Applied to files:

  • packages/router-core/src/ssr/transformStreamWithRouter.ts
📚 Learning: 2025-10-01T18:30:26.591Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.

Applied to files:

  • packages/router-core/src/ssr/transformStreamWithRouter.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Test
  • GitHub Check: Preview
🔇 Additional comments (6)
packages/router-ssr-query-core/src/index.ts (2)

41-48: Good cleanup pattern for the query cache subscription.

The changes correctly introduce lifecycle management for the query cache subscription. The guard unsubscribe?.() handles the case where cleanup might be called before the subscription is set, and resetting to undefined prevents double-unsubscription.


77-77: LGTM!

Storing the unsubscribe function enables proper cleanup on render completion.

packages/router-core/src/ssr/transformStreamWithRouter.ts (4)

107-110: Good defensive guard for the cleanup function.

Adding the check before invoking stopListeningToInjectedHtml prevents potential runtime errors if the listener hasn't been set up or was already cleaned up.


140-146: Clean refactor of the listener setup.

Processing already-injected HTML before subscribing to new events, followed by using a named handler function, improves readability and maintainability.


148-170: Consider potential race condition with concurrent injections.

The function processes all promises in injectedHtml, then clears the array on line 169. If a new promise is added to injectedHtml while the forEach is executing (but before line 169), that promise would be cleared without being processed.

In typical server-side single-threaded execution this is unlikely, but if injectedHtml can be mutated asynchronously (e.g., from within a promise callback), consider copying the array before iterating:

 function handleInjectedHtml() {
-  router.serverSsr!.injectedHtml.forEach((promise) => {
+  const promises = router.serverSsr!.injectedHtml
+  router.serverSsr!.injectedHtml = []
+  promises.forEach((promise) => {
     processingCount++
     // ...
   })
-  router.serverSsr!.injectedHtml = []
 }

184-189: Good cleanup in finally block.

This ensures the listener is properly unsubscribed regardless of whether the promise resolves or rejects, preventing memory leaks.


Comment @coderabbitai help to get the list of available commands and usage tips.

@nx-cloud
Copy link

nx-cloud bot commented Nov 25, 2025

View your CI Pipeline Execution ↗ for commit 9123be7

Command Status Duration Result
nx affected --targets=test:eslint,test:unit,tes... ✅ Succeeded 14m 37s View ↗
nx run-many --target=build --exclude=examples/*... ✅ Succeeded 1m 27s View ↗

☁️ Nx Cloud last updated this comment at 2025-11-25 20:15:00 UTC

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 25, 2025

More templates

@tanstack/arktype-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/arktype-adapter@5965

@tanstack/directive-functions-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/directive-functions-plugin@5965

@tanstack/eslint-plugin-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/eslint-plugin-router@5965

@tanstack/history

npm i https://pkg.pr.new/TanStack/router/@tanstack/history@5965

@tanstack/nitro-v2-vite-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/nitro-v2-vite-plugin@5965

@tanstack/react-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router@5965

@tanstack/react-router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router-devtools@5965

@tanstack/react-router-ssr-query

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router-ssr-query@5965

@tanstack/react-start

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start@5965

@tanstack/react-start-client

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-client@5965

@tanstack/react-start-server

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-server@5965

@tanstack/router-cli

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-cli@5965

@tanstack/router-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-core@5965

@tanstack/router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-devtools@5965

@tanstack/router-devtools-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-devtools-core@5965

@tanstack/router-generator

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-generator@5965

@tanstack/router-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-plugin@5965

@tanstack/router-ssr-query-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-ssr-query-core@5965

@tanstack/router-utils

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-utils@5965

@tanstack/router-vite-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-vite-plugin@5965

@tanstack/server-functions-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/server-functions-plugin@5965

@tanstack/solid-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-router@5965

@tanstack/solid-router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-router-devtools@5965

@tanstack/solid-router-ssr-query

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-router-ssr-query@5965

@tanstack/solid-start

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start@5965

@tanstack/solid-start-client

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-client@5965

@tanstack/solid-start-server

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-server@5965

@tanstack/start-client-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-client-core@5965

@tanstack/start-plugin-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-plugin-core@5965

@tanstack/start-server-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-server-core@5965

@tanstack/start-static-server-functions

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-static-server-functions@5965

@tanstack/start-storage-context

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-storage-context@5965

@tanstack/valibot-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/valibot-adapter@5965

@tanstack/virtual-file-routes

npm i https://pkg.pr.new/TanStack/router/@tanstack/virtual-file-routes@5965

@tanstack/zod-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/zod-adapter@5965

commit: 9123be7

@schiller-manuel schiller-manuel merged commit c78fc25 into main Nov 25, 2025
6 checks passed
@schiller-manuel schiller-manuel deleted the cleanup-refs branch November 25, 2025 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants