fix(rsc): remove outdated stale hmr css references#7219
Open
jantimon wants to merge 8 commits intoTanStack:mainfrom
Open
fix(rsc): remove outdated stale hmr css references#7219jantimon wants to merge 8 commits intoTanStack:mainfrom
jantimon wants to merge 8 commits intoTanStack:mainfrom
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 ignored due to path filters (1)
📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExtracted CSS preinit logic into a new helper and added a complete end-to-end CSS HMR test workspace (router, routes, test components, server functions, Playwright config, Vite and TS configs, and tests) for RSC HMR scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Tester as Test Runner
participant DevServer as Dev Server (Vite)
participant FS as File System
participant Browser as Browser
participant RSC as RSC Renderer
Tester->>DevServer: start (pnpm dev:e2e)
DevServer->>Browser: serve page (baseURL)
Browser->>RSC: request SSR (initial render)
RSC-->>Browser: HTML + preinit hints
Tester->>FS: edit CSS file
FS-->>DevServer: file change event
DevServer->>Browser: HMR update / module reload
Browser->>RSC: apply new styles / re-render
Browser-->>Tester: updated computed styles observed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
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)
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
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 (1)
packages/react-start-rsc/tests/preinitCssHrefs.test.ts (1)
31-93: Good coverage — consider one additional assertion for HMR skip ordering.The suite covers the important cases (positive match, negative match, mixed, undefined/empty,
ReadonlySet). ThetoHaveBeenNthCalledWithassertions on lines 67-74 already implicitly verify that cache-busted entries don't shift the call order, so this is solid.Optional nit: the
beforeEach/afterEachmockRestorepattern works, butvi.restoreAllMocks()in a top-levelafterEach(or configuringrestoreMocks: truein vitest config) would let you drop thepreinitSpyvariable declaration and the explicit restore. Not a blocker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-start-rsc/tests/preinitCssHrefs.test.ts` around lines 31 - 93, Add an explicit assertion that cache-busted HMR-style hrefs are skipped without affecting the relative ordering of non-cache-busted hrefs: update the mixed-list test that calls preinitCssHrefs(...) to include an assertion on the exact call order of ReactDOM.preinit (preinitSpy) for the non-cache-busted entries so order is validated (or add a new test that supplies interleaved cache-busted and non-cache-busted hrefs and asserts preinitSpy was called in the original relative order). Optionally simplify mock cleanup by replacing the beforeEach/afterEach preinitSpy + preinitSpy.mockRestore() pattern with a single afterEach(() => vi.restoreAllMocks()) and remove the preinitSpy variable if you prefer.
🤖 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-start-rsc/tests/preinitCssHrefs.test.ts`:
- Around line 31-93: Add an explicit assertion that cache-busted HMR-style hrefs
are skipped without affecting the relative ordering of non-cache-busted hrefs:
update the mixed-list test that calls preinitCssHrefs(...) to include an
assertion on the exact call order of ReactDOM.preinit (preinitSpy) for the
non-cache-busted entries so order is validated (or add a new test that supplies
interleaved cache-busted and non-cache-busted hrefs and asserts preinitSpy was
called in the original relative order). Optionally simplify mock cleanup by
replacing the beforeEach/afterEach preinitSpy + preinitSpy.mockRestore() pattern
with a single afterEach(() => vi.restoreAllMocks()) and remove the preinitSpy
variable if you prefer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9fe7e78c-fd22-4676-962f-e436b8c8a27e
📒 Files selected for processing (4)
packages/react-start-rsc/src/CompositeComponent.tsxpackages/react-start-rsc/src/RscNodeRenderer.tsxpackages/react-start-rsc/src/preinitCssHrefs.tspackages/react-start-rsc/tests/preinitCssHrefs.test.ts
Contributor
|
View your CI Pipeline Execution ↗ for commit 066c658
☁️ Nx Cloud last updated this comment at |
Contributor
|
does this still update css then in dev upon hmr? if yes, how? |
Contributor
|
w.r.t. to e2e tests: |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
e2e/react-start/rsc-hmr/tests/rsc-hmr-css.spec.ts (1)
90-110: Add an assertion for the stale stylesheet-link regression.This second-edit test proves CSS still updates, but not that old cache-busted
<link rel="stylesheet">entries stop accumulating. Please assert the relevant stylesheet link count/hrefs in<head>remain bounded after repeated edits so the test fails on the original leak.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/react-start/rsc-hmr/tests/rsc-hmr-css.spec.ts` around lines 90 - 110, After the two editCss steps in the test `${c.label}: a second css change in the same file hot-updates the style`, add assertions that the document head does not accumulate stale cache-busted stylesheet links: query the head for <link rel="stylesheet"> elements (via page.evaluate or page.locator) and assert the count remains bounded (e.g., equals the expected number) and that their hrefs are the current/expected href(s) rather than many old cache-busted variants; place these checks after the final expect(page.getByTestId(c.titleTestId)).toHaveCSS(...) so the test fails if stale `<link rel="stylesheet">` entries leak after calling editCss twice. Ensure you reference the same c/titleTestId context as used in this test and use page.* helpers (page.evaluate or page.locator) to inspect head links.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/react-start/rsc-hmr/package.json`:
- Line 4: The package.json currently declares "sideEffects": false which causes
Vite/webpack to tree-shake CSS imports used by GlobalCssCard.tsx and
CssModulesCard.tsx; update package.json to preserve CSS side-effects by
replacing the boolean with an array of patterns (e.g., ["*.css", "**/*.css"] or
the specific CSS module paths) or set "sideEffects": true so CSS imports are not
removed during build, ensuring GlobalCssCard.tsx and CssModulesCard.tsx CSS
remains included.
In `@e2e/react-start/rsc-hmr/tests/hydration.ts`:
- Line 1: The import currently mixes a value import and an inline type specifier
("expect, type Page from '@playwright/test'"), which ESLint flags; change it to
two imports: keep the value import for "expect" from '@playwright/test' and add
a separate type-only import for "Page" (e.g., "import type { Page } from
'@playwright/test'") so the "expect" and "Page" symbols are imported separately
and satisfy member ordering and type-import rules.
In `@e2e/react-start/rsc-hmr/tests/rsc-hmr-css.spec.ts`:
- Around line 1-5: Reorder the import statements so Node built-ins come before
package imports: move "import { readFile, writeFile } from 'node:fs/promises'"
and "import path from 'node:path'" above the third‑party imports (e.g., "import
{ expect } from '@playwright/test'" and "import { test } from
'@tanstack/router-e2e-utils'"), keeping the local relative import "import {
waitForHydration } from './hydration'" last; preserve the same named symbols
(expect, test, readFile, writeFile, path, waitForHydration) and their usage.
---
Nitpick comments:
In `@e2e/react-start/rsc-hmr/tests/rsc-hmr-css.spec.ts`:
- Around line 90-110: After the two editCss steps in the test `${c.label}: a
second css change in the same file hot-updates the style`, add assertions that
the document head does not accumulate stale cache-busted stylesheet links: query
the head for <link rel="stylesheet"> elements (via page.evaluate or
page.locator) and assert the count remains bounded (e.g., equals the expected
number) and that their hrefs are the current/expected href(s) rather than many
old cache-busted variants; place these checks after the final
expect(page.getByTestId(c.titleTestId)).toHaveCSS(...) so the test fails if
stale `<link rel="stylesheet">` entries leak after calling editCss twice. Ensure
you reference the same c/titleTestId context as used in this test and use page.*
helpers (page.evaluate or page.locator) to inspect head links.
🪄 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: 799e91c2-3377-4117-b130-eb55afc63c02
📒 Files selected for processing (23)
e2e/react-start/rsc-hmr/.gitignoree2e/react-start/rsc-hmr/eslint.config.jse2e/react-start/rsc-hmr/package.jsone2e/react-start/rsc-hmr/playwright.config.tse2e/react-start/rsc-hmr/src/routeTree.gen.tse2e/react-start/rsc-hmr/src/router.tsxe2e/react-start/rsc-hmr/src/routes/__root.tsxe2e/react-start/rsc-hmr/src/routes/index.tsxe2e/react-start/rsc-hmr/src/routes/rsc-hmr-css-modules.tsxe2e/react-start/rsc-hmr/src/routes/rsc-hmr-global-css.tsxe2e/react-start/rsc-hmr/src/utils/CssModulesCard.module.csse2e/react-start/rsc-hmr/src/utils/CssModulesCard.tsxe2e/react-start/rsc-hmr/src/utils/GlobalCssCard.csse2e/react-start/rsc-hmr/src/utils/GlobalCssCard.tsxe2e/react-start/rsc-hmr/src/utils/cssModulesCardServerComponent.tsxe2e/react-start/rsc-hmr/src/utils/globalCssCardServerComponent.tsxe2e/react-start/rsc-hmr/tests/hydration.tse2e/react-start/rsc-hmr/tests/rsc-hmr-css.spec.tse2e/react-start/rsc-hmr/tests/setup/global.setup.tse2e/react-start/rsc-hmr/tests/setup/global.teardown.tse2e/react-start/rsc-hmr/tsconfig.jsone2e/react-start/rsc-hmr/vite.config.tspackages/react-start-rsc/src/preinitCssHrefs.ts
✅ Files skipped from review due to trivial changes (7)
- e2e/react-start/rsc-hmr/.gitignore
- e2e/react-start/rsc-hmr/src/utils/GlobalCssCard.css
- e2e/react-start/rsc-hmr/src/utils/CssModulesCard.module.css
- e2e/react-start/rsc-hmr/tsconfig.json
- e2e/react-start/rsc-hmr/src/utils/GlobalCssCard.tsx
- e2e/react-start/rsc-hmr/eslint.config.js
- e2e/react-start/rsc-hmr/src/routeTree.gen.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-start-rsc/src/preinitCssHrefs.ts
Author
|
Author
|
The HMR e2e tests in this branch are blocked by vitejs/vite-plugin-react#1188 @hi-ogawa could you please take a look at both PRs and let me know what you think about the fix and the e2e tests? |
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.
follow-up to vitejs/vite-plugin-react#1188 on the TanStack Router side where @schiller-manuel was already mentioned
While debugging CSS HMR in TanStack Start RSC (https://tanstack.com/blog/react-server-components) I noticed a second, independent problem:
<link rel="stylesheet">tags pile up in<head>across HMR edits.. Every save appends another<link>and none of them get cleaned upCompositeComponentandRscNodeRendererboth walk thecssHrefsset reported by the RSC Flight stream and call:Obviously this is a dev only problem. Vite appends an HMR cache-bust (
?t=<lastHMRTimestamp>) to every CSS href on every edit (see vite-plugin-react #1188. Each edit therefore callspreinitwith a different href which adds a new<link>to<head>instead of replacing the previous oneA simple possible fix would be to skip these HMR css requests by excluding css files with
/[?&]t=\d+/as Vite only emits that query in dev so:One open question: can we somehow add HMR e2e tests to verify:
Summary by CodeRabbit
Refactor
New Features
Tests