Revert #7524#7533
Merged
Merged
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)
📝 WalkthroughWalkthroughThis PR makes TSR client teardown immediate when both hydration and stream-end are true (deleting ChangesTSR Teardown Behavior Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 00b5045
☁️ Nx Cloud last updated this comment at |
Contributor
🚀 Changeset Version Preview1 package(s) bumped directly, 22 bumped as dependents. 🟩 Patch bumps
|
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
🧹 Nitpick comments (1)
packages/router-core/tests/tsr-script-teardown.test.ts (1)
11-14: ⚡ Quick winReplace the
anywindow casts with a typed test-global shape.These assertions currently sidestep TS safety with repeated
(window as any). A small test-onlyWindowaugmentation keeps the file strict without changing behavior.♻️ Suggested typing cleanup
type TsrBootstrap = { h: () => void e: () => void c: () => void } + +declare global { + interface Window { + $_TSR?: TsrBootstrap + $R?: { + tsr?: Array<unknown> + } + } +} // Assign `self.$_TSR`. function installBootstrap(): TsrBootstrap { new Function(minifiedTsrBootStrapScript)() - return (window as any).$_TSR + return window.$_TSR! } @@ beforeEach(() => { - ;(window as any).$R = { tsr: [] } - delete (window as any).$_TSR + window.$R = { tsr: [] } + delete window.$_TSR }) @@ afterEach(() => { - delete (window as any).$_TSR - delete (window as any).$R + delete window.$_TSR + delete window.$R setReadyState('complete') }) @@ - expect((window as any).$_TSR).toBeDefined() + expect(window.$_TSR).toBeDefined() @@ - expect((window as any).$_TSR).toBeUndefined() + expect(window.$_TSR).toBeUndefined() @@ - expect((window as any).$_TSR).toBeUndefined() - expect((window as any).$R.tsr).toBeUndefined() + expect(window.$_TSR).toBeUndefined() + expect(window.$R?.tsr).toBeUndefined()As per coding guidelines,
**/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety.Also applies to: 25-31, 40-54
🤖 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/tsr-script-teardown.test.ts` around lines 11 - 14, The test uses (window as any).$_TSR which bypasses TS safety; add a test-only global Window augmentation (e.g., declare global { interface Window { $_TSR?: TsrBootstrap } }) at the top of the test file and replace all (window as any).$_TSR occurrences with the typed access window.$_TSR (and update installBootstrap to return window.$_TSR as TsrBootstrap). Ensure the augmentation is scoped to the test file so other code is unaffected.
🤖 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 @.changeset/solid-symbols-sink.md:
- Line 5: The release note is misleading: it says "delete $_TSR immediately on
stream end" but the code waits for both hydration and stream end before removing
$_TSR; update the changelog entry to reflect the actual behavior by stating that
$_TSR is deleted after stream end and hydration complete (or after both
conditions are met) rather than "immediately on stream end", and mention the
combined condition involving hydration and stream end (reference symbol $_TSR
and the hydration/stream-end condition) so the summary accurately matches the
implementation.
In `@packages/router-core/tests/tsr-script-teardown.test.ts`:
- Around line 35-55: Add a regression test for document.readyState ===
'loading': use setReadyState('loading'), call installBootstrap() to get tsr,
invoke tsr.h() and then tsr.e() and assert that (window as any).$_TSR and
(window as any).$R.tsr remain defined (teardown deferred), then simulate the
document finishing parse by setReadyState('complete') and dispatching a
DOMContentLoaded event (or call tsr.e() again) and finally assert both (window
as any).$_TSR and (window as any).$R.tsr are undefined; reference the helpers
setReadyState, installBootstrap and the tsr.h()/tsr.e() methods to locate where
to add the new test.
---
Nitpick comments:
In `@packages/router-core/tests/tsr-script-teardown.test.ts`:
- Around line 11-14: The test uses (window as any).$_TSR which bypasses TS
safety; add a test-only global Window augmentation (e.g., declare global {
interface Window { $_TSR?: TsrBootstrap } }) at the top of the test file and
replace all (window as any).$_TSR occurrences with the typed access window.$_TSR
(and update installBootstrap to return window.$_TSR as TsrBootstrap). Ensure the
augmentation is scoped to the test file so other code is unaffected.
🪄 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: fc2aaa50-a868-40c1-add1-b04489a68719
📒 Files selected for processing (3)
.changeset/solid-symbols-sink.mdpackages/router-core/src/ssr/tsrScript.tspackages/router-core/tests/tsr-script-teardown.test.ts
| '@tanstack/router-core': patch | ||
| --- | ||
|
|
||
| delete $\_TSR immediately on stream end |
Contributor
There was a problem hiding this comment.
Clarify the release note timing.
The implementation still waits for both hydration and stream end before deleting $_TSR, so “immediately on stream end” is a bit too strong. Please align the note with the actual condition to avoid misleading the patch summary.
🤖 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 @.changeset/solid-symbols-sink.md at line 5, The release note is misleading:
it says "delete $_TSR immediately on stream end" but the code waits for both
hydration and stream end before removing $_TSR; update the changelog entry to
reflect the actual behavior by stating that $_TSR is deleted after stream end
and hydration complete (or after both conditions are met) rather than
"immediately on stream end", and mention the combined condition involving
hydration and stream end (reference symbol $_TSR and the hydration/stream-end
condition) so the summary accurately matches the implementation.
Comment on lines
+35
to
+55
| test('does not tear down until both hydrated and streamEnded', () => { | ||
| setReadyState('complete') | ||
| const tsr = installBootstrap() | ||
|
|
||
| tsr.h() | ||
| expect((window as any).$_TSR).toBeDefined() | ||
|
|
||
| tsr.e() | ||
| expect((window as any).$_TSR).toBeUndefined() | ||
| }) | ||
|
|
||
| test('tears down immediately when the document is already parsed', () => { | ||
| setReadyState('complete') | ||
| const tsr = installBootstrap() | ||
|
|
||
| tsr.h() | ||
| tsr.e() | ||
|
|
||
| expect((window as any).$_TSR).toBeUndefined() | ||
| expect((window as any).$R.tsr).toBeUndefined() | ||
| }) |
Contributor
There was a problem hiding this comment.
Add a readyState === 'loading' regression case.
Both tests force document.readyState to 'complete', so the pre-revert DOMContentLoaded-deferred implementation from #7524 would still pass this suite. That means the reverted path is not actually protected here.
🧪 Minimal coverage fix
- test('tears down immediately when the document is already parsed', () => {
- setReadyState('complete')
+ test('tears down immediately after stream end even while the document is loading', () => {
+ setReadyState('loading')
const tsr = installBootstrap()
tsr.h()
tsr.e()
expect((window as any).$_TSR).toBeUndefined()
expect((window as any).$R.tsr).toBeUndefined()
})📝 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.
Suggested change
| test('does not tear down until both hydrated and streamEnded', () => { | |
| setReadyState('complete') | |
| const tsr = installBootstrap() | |
| tsr.h() | |
| expect((window as any).$_TSR).toBeDefined() | |
| tsr.e() | |
| expect((window as any).$_TSR).toBeUndefined() | |
| }) | |
| test('tears down immediately when the document is already parsed', () => { | |
| setReadyState('complete') | |
| const tsr = installBootstrap() | |
| tsr.h() | |
| tsr.e() | |
| expect((window as any).$_TSR).toBeUndefined() | |
| expect((window as any).$R.tsr).toBeUndefined() | |
| }) | |
| test('does not tear down until both hydrated and streamEnded', () => { | |
| setReadyState('complete') | |
| const tsr = installBootstrap() | |
| tsr.h() | |
| expect((window as any).$_TSR).toBeDefined() | |
| tsr.e() | |
| expect((window as any).$_TSR).toBeUndefined() | |
| }) | |
| test('tears down immediately after stream end even while the document is loading', () => { | |
| setReadyState('loading') | |
| const tsr = installBootstrap() | |
| tsr.h() | |
| tsr.e() | |
| expect((window as any).$_TSR).toBeUndefined() | |
| expect((window as any).$R.tsr).toBeUndefined() | |
| }) |
🤖 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/tsr-script-teardown.test.ts` around lines 35 - 55,
Add a regression test for document.readyState === 'loading': use
setReadyState('loading'), call installBootstrap() to get tsr, invoke tsr.h() and
then tsr.e() and assert that (window as any).$_TSR and (window as any).$R.tsr
remain defined (teardown deferred), then simulate the document finishing parse
by setReadyState('complete') and dispatching a DOMContentLoaded event (or call
tsr.e() again) and finally assert both (window as any).$_TSR and (window as
any).$R.tsr are undefined; reference the helpers setReadyState, installBootstrap
and the tsr.h()/tsr.e() methods to locate where to add the new test.
Merging this PR will improve performance by 6.07%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
Footnotes
|
| } | ||
|
|
||
| function setReadyState(value: DocumentReadyState) { | ||
| Object.defineProperty(document, 'readyState', { |
Collaborator
There was a problem hiding this comment.
why do we need to test for readyState? streaming is independent from that
Member
Author
There was a problem hiding this comment.
we don't need it now that it's immediate again - I've removed it.
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
2 participants
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.
Revert
Summary by CodeRabbit
Bug Fixes
Tests