fix(solid-router): avoid HeadContent remounts on history.replaceState#6998
Merged
fix(solid-router): avoid HeadContent remounts on history.replaceState#6998
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMemoizes Solid's Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 0e262ff
☁️ 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
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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/solid-router/tests/Scripts.test.tsx`:
- Around line 248-264: Replace the unsafe cast "as any" on the router.ssr
assignment with a concrete Manifest type: import Manifest from
"@tanstack/router-core" (or use the satisfies operator) and change the
assignment so it satisfies { ssr: { manifest: Manifest } } for the object
constructed for router.ssr (the block that builds manifest.routes for
[rootRoute.id] with assets). This preserves type safety for the manifest used by
the test while keeping the same structure for router.ssr and the entry keyed by
rootRoute.id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 363bcdfb-b88c-4247-875c-8325b80f815a
📒 Files selected for processing (2)
packages/solid-router/src/headContentUtils.tsxpackages/solid-router/tests/Scripts.test.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/vue-router/tests/Scripts.test.tsx (1)
256-272: Keep the SSR manifest fixture typed here too.This repeats the same type escape pattern as the React coverage:
as anyonrouter.ssrplus a cast onlocation.state. That makes the regression less trustworthy because the synthetic fixture can drift away from the real SSR/state contract without TS catching it.♻️ Example of the same typed-fixture pattern
- router.ssr = { - manifest: { - routes: { - [rootRoute.id]: { - assets: [ - { - tag: 'link', - attrs: { - rel: 'stylesheet', - href: '/main.css', - }, - }, - ], - }, - }, - }, - } as any + const manifest = { + routes: { + [rootRoute.id]: { + assets: [ + { + tag: 'link', + attrs: { + rel: 'stylesheet', + href: '/main.css', + }, + }, + ], + }, + }, + } satisfies NonNullable<NonNullable<typeof router.ssr>['manifest']> + + router.ssr = { ...(router.ssr ?? {}), manifest } await waitFor(() => { - expect( - (router.state.location.state as { slideId?: string }).slideId, - ).toBe('slide-2') + expect(router.state.location.state).toMatchObject({ + slideId: 'slide-2', + }) })As per coding guidelines,
**/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety.Also applies to: 294-297, 345-361
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vue-router/tests/Scripts.test.tsx` around lines 256 - 272, The test fixture currently uses an unsafe `as any` on `router.ssr` (and similar on `location.state`), which defeats TypeScript checks; replace the `as any` casts by constructing the fixture with the proper SSR/state types (e.g., import or reference the library's SSR manifest/interface and cast using that exact type) so `router.ssr = { manifest: { routes: { [rootRoute.id]: { assets: [...] } } } }` is typed as the real SSR manifest type; do the same for the `location.state` fixture so both `router.ssr`, `manifest`, `routes`, `rootRoute.id`, and `location.state` conform to the actual interfaces instead of using `any`.packages/react-router/tests/Scripts.test.tsx (1)
383-399: Avoid type escapes in the new regression fixture.These tests bypass the exact contract they are supposed to lock down with
as anyand an ad-hoc cast onlocation.state, so manifest/state drift can compile cleanly and quietly weaken the regression. Please keep the synthetic manifest and state assertion typed.♻️ Example of keeping the fixture typed
- router.ssr = { - manifest: { - routes: { - [rootRoute.id]: { - assets: [ - { - tag: 'link', - attrs: { - rel: 'stylesheet', - href: '/main.css', - }, - }, - ], - }, - }, - }, - } as any + const manifest = { + routes: { + [rootRoute.id]: { + assets: [ + { + tag: 'link', + attrs: { + rel: 'stylesheet', + href: '/main.css', + }, + }, + ], + }, + }, + } satisfies NonNullable<NonNullable<typeof router.ssr>['manifest']> + + router.ssr = { ...(router.ssr ?? {}), manifest } await waitFor(() => { - expect( - (router.state.location.state as { slideId?: string }).slideId, - ).toBe('slide-2') + expect(router.state.location.state).toMatchObject({ + slideId: 'slide-2', + }) })As per coding guidelines,
**/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety.Also applies to: 419-422, 468-484
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/vue-router/tests/Scripts.test.tsx`:
- Around line 256-272: The test fixture currently uses an unsafe `as any` on
`router.ssr` (and similar on `location.state`), which defeats TypeScript checks;
replace the `as any` casts by constructing the fixture with the proper SSR/state
types (e.g., import or reference the library's SSR manifest/interface and cast
using that exact type) so `router.ssr = { manifest: { routes: { [rootRoute.id]:
{ assets: [...] } } } }` is typed as the real SSR manifest type; do the same for
the `location.state` fixture so both `router.ssr`, `manifest`, `routes`,
`rootRoute.id`, and `location.state` conform to the actual interfaces instead of
using `any`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 590e07e6-6579-469e-adcb-236f042a861c
📒 Files selected for processing (2)
packages/react-router/tests/Scripts.test.tsxpackages/vue-router/tests/Scripts.test.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/vue-router/tests/Scripts.test.tsx (1)
282-309: Optional: extract shared stylesheet assertions into a local helper.The
getStylesheetLinklookup and uniqueness assertion are repeated; a tiny helper would reduce duplication and keep test intent even clearer.Also applies to: 357-382
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vue-router/tests/Scripts.test.tsx` around lines 282 - 309, Extract the repeated DOM queries into a small local helper and reuse it in both places (lines around the existing getStylesheetLink usage and the similar block at 357-382); specifically keep the existing getStylesheetLink logic (querySelectorAll('link[rel="stylesheet"]') and href === '/main.css') or rename it to a clearer helper like getMainStylesheetLink(), and add a companion assertion helper (e.g., assertSingleMainStylesheet()) that asserts the returned element is an HTMLLinkElement and that only one link with href '/main.css' exists; replace the duplicated inline queries and uniqueness checks with calls to these helpers in the test blocks that currently reference getStylesheetLink and the Array.from(...).filter(...) uniqueness assertion.packages/solid-router/tests/Scripts.test.tsx (1)
179-212: Optional: extract shared head-link query/cleanup helpers.Both tests repeat the same stylesheet lookup and removal logic; local helper extraction would reduce repetition and keep intent focused.
Also applies to: 351-386
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/solid-router/tests/Scripts.test.tsx` around lines 179 - 212, Extract the repeated stylesheet lookup and cleanup logic into shared helper functions (e.g., getStylesheetLink() and removeStylesheetLinks(href: string)) and replace the duplicated blocks in the test (the getStylesheetLink definition, the final forEach cleanup, and the similar code around lines 351-386) with calls to those helpers; keep the helpers scoped to the test file (top of the describe or test suite) so tests call getStylesheetLink() to assert presence and removeStylesheetLinks('/main.css') in the finally cleanup.packages/react-router/tests/Scripts.test.tsx (1)
409-434: Optional: de-duplicate repeated stylesheet lookup/assertion blocks.A shared helper in this describe block would trim repetition and make future maintenance easier without changing behavior.
Also applies to: 480-503
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-router/tests/Scripts.test.tsx` around lines 409 - 434, Create a shared helper function in the test's describe block to centralize the repeated stylesheet lookup/assertions: move the inline getStylesheetLink logic into a single helper (e.g., getStylesheetLink()) used by the tests that currently duplicate it (the block around the click/assert and the other similar block at lines 480-503). Update tests that call the inline Array.from(document.querySelectorAll('link[rel="stylesheet"]'))/... checks to call the helper and reuse a single assertion helper for "expect(...).toBeInstanceOf(HTMLLinkElement)" and the length check, so both the initialLink capture, post-click identity check, and the duplicate-count assertion use the centralized helper instead of repeating the DOM query.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react-router/tests/Scripts.test.tsx`:
- Around line 409-434: Create a shared helper function in the test's describe
block to centralize the repeated stylesheet lookup/assertions: move the inline
getStylesheetLink logic into a single helper (e.g., getStylesheetLink()) used by
the tests that currently duplicate it (the block around the click/assert and the
other similar block at lines 480-503). Update tests that call the inline
Array.from(document.querySelectorAll('link[rel="stylesheet"]'))/... checks to
call the helper and reuse a single assertion helper for
"expect(...).toBeInstanceOf(HTMLLinkElement)" and the length check, so both the
initialLink capture, post-click identity check, and the duplicate-count
assertion use the centralized helper instead of repeating the DOM query.
In `@packages/solid-router/tests/Scripts.test.tsx`:
- Around line 179-212: Extract the repeated stylesheet lookup and cleanup logic
into shared helper functions (e.g., getStylesheetLink() and
removeStylesheetLinks(href: string)) and replace the duplicated blocks in the
test (the getStylesheetLink definition, the final forEach cleanup, and the
similar code around lines 351-386) with calls to those helpers; keep the helpers
scoped to the test file (top of the describe or test suite) so tests call
getStylesheetLink() to assert presence and removeStylesheetLinks('/main.css') in
the finally cleanup.
In `@packages/vue-router/tests/Scripts.test.tsx`:
- Around line 282-309: Extract the repeated DOM queries into a small local
helper and reuse it in both places (lines around the existing getStylesheetLink
usage and the similar block at 357-382); specifically keep the existing
getStylesheetLink logic (querySelectorAll('link[rel="stylesheet"]') and href ===
'/main.css') or rename it to a clearer helper like getMainStylesheetLink(), and
add a companion assertion helper (e.g., assertSingleMainStylesheet()) that
asserts the returned element is an HTMLLinkElement and that only one link with
href '/main.css' exists; replace the duplicated inline queries and uniqueness
checks with calls to these helpers in the test blocks that currently reference
getStylesheetLink and the Array.from(...).filter(...) uniqueness assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aca8d516-7ea3-45be-8202-5bc7186ba548
📒 Files selected for processing (3)
packages/react-router/tests/Scripts.test.tsxpackages/solid-router/tests/Scripts.test.tsxpackages/vue-router/tests/Scripts.test.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/solid-router/tests/Scripts.test.tsx (1)
232-241: Extract repeated stylesheet cleanup into a small helper.The
/main.csscleanup logic is duplicated across bothfinallyblocks. A shared local helper would reduce repetition and make future test updates safer.Refactor sketch
+const removeMainCssLinks = () => { + document.head.querySelectorAll('link[rel="stylesheet"]').forEach((link) => { + if (link.getAttribute('href') === '/main.css') { + link.remove() + } + }) +} ... - document.head - .querySelectorAll('link[rel="stylesheet"]') - .forEach((link) => { - if (link.getAttribute('href') === '/main.css') { - link.remove() - } - }) + removeMainCssLinks() ... - document.head - .querySelectorAll('link[rel="stylesheet"]') - .forEach((link) => { - if (link.getAttribute('href') === '/main.css') { - link.remove() - } - }) + removeMainCssLinks()Also applies to: 407-415
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/solid-router/tests/Scripts.test.tsx` around lines 232 - 241, Extract the duplicated "/main.css" removal logic into a small local helper (e.g., removeMainCssLink) inside the test file and call it from both finally blocks instead of repeating the querySelectorAll/forEach block; locate the duplicated code around the finally blocks that reference observer and history and replace each repeated snippet with a call to the new helper so both cleanup sites share the same implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/solid-router/tests/Scripts.test.tsx`:
- Around line 232-241: Extract the duplicated "/main.css" removal logic into a
small local helper (e.g., removeMainCssLink) inside the test file and call it
from both finally blocks instead of repeating the querySelectorAll/forEach
block; locate the duplicated code around the finally blocks that reference
observer and history and replace each repeated snippet with a call to the new
helper so both cleanup sites share the same implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06f04c35-55d2-4b75-a682-5c0e8be5fea3
📒 Files selected for processing (1)
packages/solid-router/tests/Scripts.test.tsx
birkskyum
approved these changes
Mar 20, 2026
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.
Summary
HeadContenttags so identical manifest assets stay mounted across history state updateswindow.history.replaceState(...)Screenshots
Before (warning flashing lights):
Screen.Recording.2026-03-20.at.20.50.02.mov
After:
Screen.Recording.2026-03-20.at.20.49.11.mov
Testing
CI=1 NX_DAEMON=false pnpm nx run @tanstack/solid-router:test:unit --outputStyle=stream --skipRemoteCacheCI=1 NX_DAEMON=false pnpm nx run @tanstack/solid-router:test:types --outputStyle=stream --skipRemoteCacheCI=1 NX_DAEMON=false pnpm nx run @tanstack/solid-router:test:eslint --outputStyle=stream --skipRemoteCacheSummary by CodeRabbit
Bug Fixes
Performance
Tests