fix react-router shared route css persistence on nav#7186
Merged
schiller-manuel merged 6 commits intomainfrom Apr 15, 2026
Merged
fix react-router shared route css persistence on nav#7186schiller-manuel merged 6 commits intomainfrom
schiller-manuel merged 6 commits intomainfrom
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 as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdates manifest asset resolution/deduplication and React Asset link rendering (adds stylesheet Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Router
participant Manifest
participant Asset
participant Head
Browser->>Router: navigate(path)
Router->>Manifest: request route assets
Manifest-->>Router: respond with assets (styles/scripts)
Router->>Asset: render assets
Asset->>Head: inject <link rel="stylesheet" href=... precedence='default'>
Head->>Browser: stylesheet applied
Browser->>Router: client-side sibling navigation
Router->>Manifest: resolve new route assets
Manifest-->>Router: assets (may include shared stylesheet)
Router->>Asset: reconcile/inject assets (preserve shared identity)
Asset->>Head: add/update <link> nodes
Head->>Browser: styles remain consistent
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
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 docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
View your CI Pipeline Execution ↗ for commit 5b0be55
☁️ Nx Cloud last updated this comment at |
Contributor
🚀 Changeset Version Preview1 package(s) bumped directly, 4 bumped as dependents. 🟩 Patch bumps
|
Contributor
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
remove hash workaround for dynamically imported chunks' CSS
schiller-manuel
force-pushed
the
fix-7172
branch
from
4fe6fe1 to
259bdf0
Compare
![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 (7)
e2e/react-start/start-manifest/tests/start-manifest.spec.ts (1)
335-345: Avoid hard-coding the emitted CSS chunk name in this assertion.Matching
href*="SharedCard"makes the test depend on Vite's asset naming instead of the behavior under test. A harmless chunk-renaming change would fail this even if CSS persistence still works. Capture the actual shared stylesheet href from the manifest or from the first render, then assert that exact href stays mounted once after navigating to/b.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/react-start/start-manifest/tests/start-manifest.spec.ts` around lines 335 - 345, The test currently hard-codes the CSS chunk match by using page.locator('head link[rel="stylesheet"][href*="SharedCard"]'), which couples the test to Vite's generated name; change it to capture the actual stylesheet href from the first render (e.g., read the href attribute from the locator used before navigation) and store it in a variable, then after getByTestId('nav-/b').click() and waitForURL('**/b') assert that a locator matching head link[rel="stylesheet"][href="{capturedHref}"] exists exactly once (use the capturedHref string in the second expect instead of 'SharedCard'); this ensures you assert the exact emitted href persists rather than matching a brittle partial name.packages/react-router/tests/Scripts.test.tsx (1)
30-35: Widen the helper inputs so later cases stay typed.This options shape only models string href overrides, which is why the cross-origin case later has to bypass manifest typing with an inline
as any. LetcreateTestManifestaccept the real preload/asset shapes instead of onlystringhrefs.As per coding guidelines, "
**/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety`."🤖 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 30 - 35, The helper createTestManifest currently types options.stylesheetHref and options.preloadHref as plain strings which forces callers to cast for real manifest entries; change createTestManifest's options to accept the actual manifest asset/preload shapes used across the codebase (e.g. the same types used for stylesheet and preload entries in your runtime manifest such as ManifestStylesheet/ManifestPreload or the AssetDescriptor type) instead of string hrefs, update the function to read href from those objects (e.g. option.stylesheet.href / option.preload.href) and update tests that passed strings to pass the proper shaped objects so the cross-origin test no longer needs an inline as any.packages/start-plugin-core/tests/start-manifest-plugin/manifestBuilder.test.ts (1)
230-261: Assert the collector returns the cached stylesheet objects.This currently proves only that
getStylesheetAsset()memoizes. It does not provegetChunkCssAssets()reuses those same instances. If the collector starts cloning link tags, this test still passes while the shared-link reuse behavior can regress.Proposed assertion tightening
expect(assets).toEqual([ { tag: 'link', attrs: { rel: 'stylesheet', href: '/assets/entry.css', type: 'text/css', }, }, { tag: 'link', attrs: { rel: 'stylesheet', href: '/assets/shared.css', type: 'text/css', }, }, ]) - expect(resolvers.getStylesheetAsset('shared.css')).toBe( - resolvers.getStylesheetAsset('shared.css'), - ) + expect(assets[0]).toBe(resolvers.getStylesheetAsset('entry.css')) + expect(assets[1]).toBe(resolvers.getStylesheetAsset('shared.css'))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/tests/start-manifest-plugin/manifestBuilder.test.ts` around lines 230 - 261, The test only checks getStylesheetAsset() memoization but not that createChunkCssAssetCollector.getChunkCssAssets() returns the exact same object instances; update the assertions to verify identity: after calling cssAssetCollector.getChunkCssAssets(...) capture the returned assets array and assert that specific entries (e.g., the shared stylesheet link) are the exact same instance as returned by createManifestAssetResolvers().getStylesheetAsset('shared.css') using strict identity checks (toBe) for both the shared and entry stylesheet entries so the collector cannot pass by cloning link objects; reference cssAssetCollector, getChunkCssAssets, createManifestAssetResolvers, and getStylesheetAsset in the assertion.e2e/solid-start/start-manifest/tests/start-manifest.spec.ts (2)
1-6: Reorder node imports before external packages.Same as the Vue test file—ESLint expects
node:*imports to appear before external package imports.🔧 Suggested fix
+import { readdir } from 'node:fs/promises' +import path from 'node:path' +import { pathToFileURL } from 'node:url' import { expect } from '@playwright/test' import { test } from '@tanstack/router-e2e-utils' import type { Page } from '@playwright/test' -import { readdir } from 'node:fs/promises' -import path from 'node:path' -import { pathToFileURL } from 'node:url'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/solid-start/start-manifest/tests/start-manifest.spec.ts` around lines 1 - 6, Move the Node built-in imports (readdir from 'node:fs/promises', path from 'node:path', pathToFileURL from 'node:url') to appear before external package imports; update the import order in start-manifest.spec.ts so the three node:* imports are listed above imports like '@playwright/test' and '@tanstack/router-e2e-utils' (check the top-of-file import block containing expect, test, and Page to reorder accordingly).
80-82: Use function property syntax instead of shorthand method signatures.Same as the Vue test file—use function property syntax for consistency with ESLint rules.
🔧 Suggested fix
request: { - get: (url: string) => Promise<{ ok(): boolean; text(): Promise<string> }> + get: (url: string) => Promise<{ ok: () => boolean; text: () => Promise<string> }> }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/solid-start/start-manifest/tests/start-manifest.spec.ts` around lines 80 - 82, Change the inline response type to use function property syntax instead of method shorthand: update the anonymous response shape returned by request.get so that ok and text are declared as properties (ok: () => boolean and text: () => Promise<string>) rather than shorthand methods (ok(): boolean and text(): Promise<string>); this affects the request.get return type and the ok and text members referenced in start-manifest.spec.ts.e2e/vue-start/start-manifest/tests/start-manifest.spec.ts (2)
1-6: Reorder node imports before external packages.ESLint flags that
node:*imports should appear before external package imports.🔧 Suggested fix
+import { readdir } from 'node:fs/promises' +import path from 'node:path' +import { pathToFileURL } from 'node:url' import { expect } from '@playwright/test' import { test } from '@tanstack/router-e2e-utils' import type { Page } from '@playwright/test' -import { readdir } from 'node:fs/promises' -import path from 'node:path' -import { pathToFileURL } from 'node:url'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/vue-start/start-manifest/tests/start-manifest.spec.ts` around lines 1 - 6, Move all Node built-in imports (readdir from 'node:fs/promises', path from 'node:path', pathToFileURL from 'node:url') above the external package imports (expect and Page from '@playwright/test' and test from '@tanstack/router-e2e-utils'); reorder the import block so node:* imports come first, then external packages, keeping the same specifier names and only changing order to satisfy ESLint.
84-86: Use function property syntax instead of shorthand method signatures.ESLint's
@typescript-eslint/method-signature-stylerule requires function properties instead of shorthand method signatures in type definitions.🔧 Suggested fix
request: { - get: (url: string) => Promise<{ ok(): boolean; text(): Promise<string> }> + get: (url: string) => Promise<{ ok: () => boolean; text: () => Promise<string> }> }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/vue-start/start-manifest/tests/start-manifest.spec.ts` around lines 84 - 86, The type for request.get currently uses shorthand method signatures for the returned object (ok() and text()) which violates `@typescript-eslint/method-signature-style`; update the return type of request.get to use function property syntax instead (e.g., change ok() to ok: () => boolean and text() to text: () => Promise<string>) so the request.get signature becomes get: (url: string) => Promise<{ ok: () => boolean; text: () => Promise<string> }>, adjusting the types referenced in start-manifest.spec.ts for request.get, ok, and text accordingly.
🤖 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/start-manifest/tests/start-manifest.spec.ts`:
- Around line 442-446: The wait after clicking the home nav uses a loose regex
(page.waitForURL(/\/([^/]*)(\/)?($|\?)/)) that can match other paths like
/lazy-css-lazy; replace it with an explicit wait for the root URL (e.g.
page.waitForURL('/') or a glob like page.waitForURL('**/')) immediately after
await page.getByTestId('nav-home').click(), so the home navigation completes
before calling await page.getByTestId('nav-/lazy-css-lazy').click(); update the
waitForURL call in the same test (the two occurrences around nav-home and
nav-/lazy-css-lazy) to use the explicit root match.
In `@packages/start-plugin-core/src/start-manifest-plugin/manifestBuilder.ts`:
- Around line 113-129: getAssetIdentity currently builds dedupe keys only from
href/src and type, which collapses assets that differ by important attributes;
update getAssetIdentity to include the meaningful attributes into the identity
string so deduping preserves distinct variants (for link include attrs.media,
attrs.crossOrigin, attrs.integrity, attrs.as and any other relevant attributes;
for script include attrs.async, attrs.defer, attrs.crossOrigin, attrs.integrity
and any other relevant attributes as well as children) and keep the same
delimiter pattern so appendUniqueAssets and dedupeNestedRouteManifestEntries
will treat these as distinct entries.
---
Nitpick comments:
In `@e2e/react-start/start-manifest/tests/start-manifest.spec.ts`:
- Around line 335-345: The test currently hard-codes the CSS chunk match by
using page.locator('head link[rel="stylesheet"][href*="SharedCard"]'), which
couples the test to Vite's generated name; change it to capture the actual
stylesheet href from the first render (e.g., read the href attribute from the
locator used before navigation) and store it in a variable, then after
getByTestId('nav-/b').click() and waitForURL('**/b') assert that a locator
matching head link[rel="stylesheet"][href="{capturedHref}"] exists exactly once
(use the capturedHref string in the second expect instead of 'SharedCard'); this
ensures you assert the exact emitted href persists rather than matching a
brittle partial name.
In `@e2e/solid-start/start-manifest/tests/start-manifest.spec.ts`:
- Around line 1-6: Move the Node built-in imports (readdir from
'node:fs/promises', path from 'node:path', pathToFileURL from 'node:url') to
appear before external package imports; update the import order in
start-manifest.spec.ts so the three node:* imports are listed above imports like
'@playwright/test' and '@tanstack/router-e2e-utils' (check the top-of-file
import block containing expect, test, and Page to reorder accordingly).
- Around line 80-82: Change the inline response type to use function property
syntax instead of method shorthand: update the anonymous response shape returned
by request.get so that ok and text are declared as properties (ok: () => boolean
and text: () => Promise<string>) rather than shorthand methods (ok(): boolean
and text(): Promise<string>); this affects the request.get return type and the
ok and text members referenced in start-manifest.spec.ts.
In `@e2e/vue-start/start-manifest/tests/start-manifest.spec.ts`:
- Around line 1-6: Move all Node built-in imports (readdir from
'node:fs/promises', path from 'node:path', pathToFileURL from 'node:url') above
the external package imports (expect and Page from '@playwright/test' and test
from '@tanstack/router-e2e-utils'); reorder the import block so node:* imports
come first, then external packages, keeping the same specifier names and only
changing order to satisfy ESLint.
- Around line 84-86: The type for request.get currently uses shorthand method
signatures for the returned object (ok() and text()) which violates
`@typescript-eslint/method-signature-style`; update the return type of request.get
to use function property syntax instead (e.g., change ok() to ok: () => boolean
and text() to text: () => Promise<string>) so the request.get signature becomes
get: (url: string) => Promise<{ ok: () => boolean; text: () => Promise<string>
}>, adjusting the types referenced in start-manifest.spec.ts for request.get,
ok, and text accordingly.
In `@packages/react-router/tests/Scripts.test.tsx`:
- Around line 30-35: The helper createTestManifest currently types
options.stylesheetHref and options.preloadHref as plain strings which forces
callers to cast for real manifest entries; change createTestManifest's options
to accept the actual manifest asset/preload shapes used across the codebase
(e.g. the same types used for stylesheet and preload entries in your runtime
manifest such as ManifestStylesheet/ManifestPreload or the AssetDescriptor type)
instead of string hrefs, update the function to read href from those objects
(e.g. option.stylesheet.href / option.preload.href) and update tests that passed
strings to pass the proper shaped objects so the cross-origin test no longer
needs an inline as any.
In
`@packages/start-plugin-core/tests/start-manifest-plugin/manifestBuilder.test.ts`:
- Around line 230-261: The test only checks getStylesheetAsset() memoization but
not that createChunkCssAssetCollector.getChunkCssAssets() returns the exact same
object instances; update the assertions to verify identity: after calling
cssAssetCollector.getChunkCssAssets(...) capture the returned assets array and
assert that specific entries (e.g., the shared stylesheet link) are the exact
same instance as returned by
createManifestAssetResolvers().getStylesheetAsset('shared.css') using strict
identity checks (toBe) for both the shared and entry stylesheet entries so the
collector cannot pass by cloning link objects; reference cssAssetCollector,
getChunkCssAssets, createManifestAssetResolvers, and getStylesheetAsset in the
assertion.
🪄 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: 602fd7a5-6f46-4a9e-ba4a-337485dab3e6
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (94)
.changeset/tough-bears-tease.mde2e/react-start/css-modules/src/routeTree.gen.tse2e/react-start/css-modules/src/routes/__root.tsxe2e/react-start/css-modules/src/styles/shared-widget.module.csse2e/react-start/css-modules/tests/css.prod.spec.tse2e/react-start/rsc/tests/rsc-css-modules.spec.tse2e/react-start/start-manifest/src/components/AppShell.tsxe2e/react-start/start-manifest/src/components/SharedCard.tsxe2e/react-start/start-manifest/src/components/SharedWidget.tsxe2e/react-start/start-manifest/src/components/SharedWidgetLazy.tsxe2e/react-start/start-manifest/src/routeTree.gen.tse2e/react-start/start-manifest/src/routes/a.tsxe2e/react-start/start-manifest/src/routes/b.tsxe2e/react-start/start-manifest/src/routes/lazy-css-lazy.tsxe2e/react-start/start-manifest/src/routes/lazy-css-static.tsxe2e/react-start/start-manifest/src/styles/page-a.module.csse2e/react-start/start-manifest/src/styles/page-b.module.csse2e/react-start/start-manifest/src/styles/shared-card.module.csse2e/react-start/start-manifest/src/styles/shared-widget.module.csse2e/react-start/start-manifest/tests/start-manifest.spec.tse2e/solid-start/start-manifest/package.jsone2e/solid-start/start-manifest/playwright.config.tse2e/solid-start/start-manifest/src/components/AppShell.tsxe2e/solid-start/start-manifest/src/components/SharedCard.tsxe2e/solid-start/start-manifest/src/components/SharedNestedLayout.tsxe2e/solid-start/start-manifest/src/components/SharedWidget.tsxe2e/solid-start/start-manifest/src/components/SharedWidgetLazy.tsxe2e/solid-start/start-manifest/src/routeTree.gen.tse2e/solid-start/start-manifest/src/router.tsxe2e/solid-start/start-manifest/src/routes/__root.tsxe2e/solid-start/start-manifest/src/routes/a.tsxe2e/solid-start/start-manifest/src/routes/b.tsxe2e/solid-start/start-manifest/src/routes/index.tsxe2e/solid-start/start-manifest/src/routes/lazy-css-lazy.tsxe2e/solid-start/start-manifest/src/routes/lazy-css-static.tsxe2e/solid-start/start-manifest/src/routes/r1.tsxe2e/solid-start/start-manifest/src/routes/r2.tsxe2e/solid-start/start-manifest/src/routes/r3.tsxe2e/solid-start/start-manifest/src/routes/r4.tsxe2e/solid-start/start-manifest/src/routes/r5.tsxe2e/solid-start/start-manifest/src/routes/r6.tsxe2e/solid-start/start-manifest/src/routes/shared-a.tsxe2e/solid-start/start-manifest/src/routes/shared-b.tsxe2e/solid-start/start-manifest/src/routes/shared-c.tsxe2e/solid-start/start-manifest/src/styles/page-a.module.csse2e/solid-start/start-manifest/src/styles/page-b.module.csse2e/solid-start/start-manifest/src/styles/root-shell.module.csse2e/solid-start/start-manifest/src/styles/route-one.module.csse2e/solid-start/start-manifest/src/styles/route-two.module.csse2e/solid-start/start-manifest/src/styles/shared-card.module.csse2e/solid-start/start-manifest/src/styles/shared-layout.module.csse2e/solid-start/start-manifest/src/styles/shared-widget.module.csse2e/solid-start/start-manifest/tests/start-manifest.spec.tse2e/solid-start/start-manifest/tsconfig.jsone2e/solid-start/start-manifest/vite.config.tse2e/vue-start/start-manifest/package.jsone2e/vue-start/start-manifest/playwright.config.tse2e/vue-start/start-manifest/src/components/AppShell.tsxe2e/vue-start/start-manifest/src/components/SharedCard.tsxe2e/vue-start/start-manifest/src/components/SharedNestedLayout.tsxe2e/vue-start/start-manifest/src/components/SharedWidget.tsxe2e/vue-start/start-manifest/src/components/SharedWidgetLazy.tsxe2e/vue-start/start-manifest/src/routeTree.gen.tse2e/vue-start/start-manifest/src/router.tsxe2e/vue-start/start-manifest/src/routes/__root.tsxe2e/vue-start/start-manifest/src/routes/a.tsxe2e/vue-start/start-manifest/src/routes/b.tsxe2e/vue-start/start-manifest/src/routes/index.tsxe2e/vue-start/start-manifest/src/routes/lazy-css-lazy.tsxe2e/vue-start/start-manifest/src/routes/lazy-css-static.tsxe2e/vue-start/start-manifest/src/routes/r1.tsxe2e/vue-start/start-manifest/src/routes/r2.tsxe2e/vue-start/start-manifest/src/routes/r3.tsxe2e/vue-start/start-manifest/src/routes/r4.tsxe2e/vue-start/start-manifest/src/routes/r5.tsxe2e/vue-start/start-manifest/src/routes/r6.tsxe2e/vue-start/start-manifest/src/routes/shared-a.tsxe2e/vue-start/start-manifest/src/routes/shared-b.tsxe2e/vue-start/start-manifest/src/routes/shared-c.tsxe2e/vue-start/start-manifest/src/styles/page-a.module.csse2e/vue-start/start-manifest/src/styles/page-b.module.csse2e/vue-start/start-manifest/src/styles/root-shell.module.csse2e/vue-start/start-manifest/src/styles/route-one.module.csse2e/vue-start/start-manifest/src/styles/route-two.module.csse2e/vue-start/start-manifest/src/styles/shared-card.module.csse2e/vue-start/start-manifest/src/styles/shared-layout.module.csse2e/vue-start/start-manifest/src/styles/shared-widget.module.csse2e/vue-start/start-manifest/tests/start-manifest.spec.tse2e/vue-start/start-manifest/tsconfig.jsone2e/vue-start/start-manifest/vite.config.tspackages/react-router/src/Asset.tsxpackages/react-router/tests/Scripts.test.tsxpackages/start-plugin-core/src/start-manifest-plugin/manifestBuilder.tspackages/start-plugin-core/tests/start-manifest-plugin/manifestBuilder.test.ts
💤 Files with no reviewable changes (3)
- e2e/react-start/css-modules/src/routes/__root.tsx
- e2e/react-start/css-modules/src/styles/shared-widget.module.css
- e2e/react-start/css-modules/tests/css.prod.spec.ts
✅ Files skipped from review due to trivial changes (38)
- .changeset/tough-bears-tease.md
- e2e/react-start/start-manifest/src/routes/lazy-css-static.tsx
- e2e/react-start/start-manifest/src/components/SharedWidgetLazy.tsx
- e2e/react-start/start-manifest/src/styles/shared-card.module.css
- e2e/react-start/start-manifest/src/components/SharedWidget.tsx
- e2e/solid-start/start-manifest/src/components/SharedWidgetLazy.tsx
- e2e/solid-start/start-manifest/src/styles/route-one.module.css
- e2e/react-start/start-manifest/src/components/SharedCard.tsx
- e2e/solid-start/start-manifest/src/styles/route-two.module.css
- e2e/react-start/start-manifest/src/styles/page-a.module.css
- e2e/vue-start/start-manifest/src/styles/route-two.module.css
- e2e/react-start/start-manifest/src/styles/shared-widget.module.css
- e2e/vue-start/start-manifest/src/components/SharedWidgetLazy.tsx
- e2e/solid-start/start-manifest/src/styles/shared-layout.module.css
- e2e/solid-start/start-manifest/src/styles/page-a.module.css
- e2e/solid-start/start-manifest/src/styles/shared-card.module.css
- e2e/solid-start/start-manifest/src/styles/shared-widget.module.css
- e2e/react-start/start-manifest/src/styles/page-b.module.css
- e2e/react-start/start-manifest/src/routes/a.tsx
- e2e/vue-start/start-manifest/src/styles/shared-card.module.css
- e2e/vue-start/start-manifest/src/styles/page-b.module.css
- e2e/solid-start/start-manifest/src/components/SharedWidget.tsx
- e2e/vue-start/start-manifest/src/styles/page-a.module.css
- e2e/solid-start/start-manifest/src/routes/r1.tsx
- e2e/solid-start/start-manifest/src/styles/page-b.module.css
- e2e/vue-start/start-manifest/src/styles/route-one.module.css
- e2e/vue-start/start-manifest/src/styles/shared-layout.module.css
- e2e/react-start/rsc/tests/rsc-css-modules.spec.ts
- e2e/solid-start/start-manifest/src/styles/root-shell.module.css
- e2e/vue-start/start-manifest/src/styles/root-shell.module.css
- e2e/vue-start/start-manifest/src/styles/shared-widget.module.css
- e2e/vue-start/start-manifest/package.json
- e2e/solid-start/start-manifest/tsconfig.json
- e2e/vue-start/start-manifest/tsconfig.json
- e2e/solid-start/start-manifest/package.json
- e2e/solid-start/start-manifest/src/routeTree.gen.ts
- e2e/vue-start/start-manifest/src/routeTree.gen.ts
- e2e/react-start/css-modules/src/routeTree.gen.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- e2e/react-start/start-manifest/src/routes/b.tsx
- e2e/react-start/start-manifest/src/components/AppShell.tsx
- packages/react-router/src/Asset.tsx
packages/start-plugin-core/src/start-manifest-plugin/manifestBuilder.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/react-start/start-manifest/tests/start-manifest.spec.ts (1)
360-364: Optional DRY cleanup for repeated border-color evaluation.
borderTopColorextraction is repeated in three places; a small helper would reduce duplication and make future style-assert changes safer.♻️ Suggested refactor
async function getBackgroundColor(testId: string, page: Page) { return page .getByTestId(testId) .evaluate((element) => getComputedStyle(element).backgroundColor) } + +async function getBorderTopColor(testId: string, page: Page) { + return page + .getByTestId(testId) + .evaluate((element) => getComputedStyle(element).borderTopColor) +} @@ - expect( - await widget.evaluate( - (element) => getComputedStyle(element).borderTopColor, - ), - ).toBe(SHARED_WIDGET_BORDER) + expect(await getBorderTopColor('shared-widget', page)).toBe( + SHARED_WIDGET_BORDER, + ) @@ - expect( - await widget.evaluate( - (element) => getComputedStyle(element).borderTopColor, - ), - ).toBe(SHARED_WIDGET_BORDER) + expect(await getBorderTopColor('shared-widget', page)).toBe( + SHARED_WIDGET_BORDER, + ) @@ - expect( - await widget.evaluate( - (element) => getComputedStyle(element).borderTopColor, - ), - ).toBe(SHARED_WIDGET_BORDER) + expect(await getBorderTopColor('shared-widget', page)).toBe( + SHARED_WIDGET_BORDER, + )Also applies to: 402-406, 425-427
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/react-start/start-manifest/tests/start-manifest.spec.ts` around lines 360 - 364, Create a small helper to DRY the repeated extraction of borderTopColor used in the tests (the widget.evaluate((element) => getComputedStyle(element).borderTopColor) calls) — add a function like getWidgetBorderTopColor(widget) in the test file and replace the three inline widget.evaluate usages (the occurrences around the expectations at/near the current diff and lines 402-406 and 425-427) with calls to that helper so the style access is centralized and easier to update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/react-start/start-manifest/tests/start-manifest.spec.ts`:
- Around line 360-364: Create a small helper to DRY the repeated extraction of
borderTopColor used in the tests (the widget.evaluate((element) =>
getComputedStyle(element).borderTopColor) calls) — add a function like
getWidgetBorderTopColor(widget) in the test file and replace the three inline
widget.evaluate usages (the occurrences around the expectations at/near the
current diff and lines 402-406 and 425-427) with calls to that helper so the
style access is centralized and easier to update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a5780e5d-c4ac-4a8d-be94-42d8628828b1
📒 Files selected for processing (1)
e2e/react-start/start-manifest/tests/start-manifest.spec.ts
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
1 participant
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 by CodeRabbit
Bug Fixes
New Features
Tests