Nextjs: New rsc vite framework #32412
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| export const RSC = async ({ label }: { label: string }) => <>RSC {label}</>; | ||
|
|
||
| export const Nested = async ({ children }: any) => <>Nested {children}</>; |
There was a problem hiding this comment.
style: Use proper typing for children prop instead of any. Consider { children: React.ReactNode }
| export const Nested = async ({ children }: any) => <>Nested {children}</>; | |
| export const Nested = async ({ children }: { children: React.ReactNode }) => <>Nested {children}</>; |
| @@ -0,0 +1,93 @@ | |||
| Copyright 2020 The Rubik Filtered Project Authors (https://https://github.com/NaN-xyz/Rubik-Filtered) | |||
There was a problem hiding this comment.
syntax: URL has double https:// protocol - should be https://github.com/NaN-xyz/Rubik-Filtered
| Copyright 2020 The Rubik Filtered Project Authors (https://https://github.com/NaN-xyz/Rubik-Filtered) | |
| Copyright 2020 The Rubik Filtered Project Authors (https://github.com/NaN-xyz/Rubik-Filtered) |
| @@ -0,0 +1,118 @@ | |||
| import React, { useRef, useState } from 'react'; | |||
There was a problem hiding this comment.
style: Remove unused useRef import
| import React, { useRef, useState } from 'react'; | |
| import React, { useState } from 'react'; |
| <style jsx>{` | ||
| button { | ||
| background-color: ${backgroundColor}; | ||
| } | ||
| `}</style> |
There was a problem hiding this comment.
style: styled-jsx may not work properly with React Server Components. Consider using CSS modules or external stylesheets for RSC compatibility.
|
|
||
| if (overrides?.events) { | ||
| Object.keys(routerEvents).forEach((key) => { | ||
| if (key in routerEvents) { |
There was a problem hiding this comment.
logic: Logic error: should check key in overrides.events instead of key in routerEvents to properly override event handlers
| if (key in routerEvents) { | |
| if (key in overrides.events) { |
| @@ -0,0 +1,22 @@ | |||
| import { fn } from 'storybook/test'; | |||
|
|
|||
| // biome-ignore lint/suspicious/noExplicitAny: <explanation> | |||
There was a problem hiding this comment.
style: biome-ignore comment needs specific explanation - consider 'accepts any callback type for Next.js cache compatibility'
| // biome-ignore lint/suspicious/noExplicitAny: <explanation> | |
| // biome-ignore lint/suspicious/noExplicitAny: accepts any callback type for Next.js cache compatibility |
| @@ -0,0 +1,8 @@ | |||
| { | |||
| "name": "nextjs-vite", | |||
There was a problem hiding this comment.
logic: Project name should be 'nextjs-vite-rsc' to match directory name and avoid conflicts with existing nextjs-vite framework
| "name": "nextjs-vite", | |
| "name": "nextjs-vite-rsc", |
| test: { | ||
| // This is needed until Next will update to the React 19 beta: https://github.com/vercel/next.js/pull/65058 | ||
| // In the React 19 beta ErrorBoundary errors (such as redirect) are only logged, and not thrown. | ||
| // We will also suspress console.error logs for re the console.error logs for redirect in the next framework. |
There was a problem hiding this comment.
syntax: Typo in comment: 'suspress' should be 'suppress'
| // We will also suspress console.error logs for re the console.error logs for redirect in the next framework. | |
| // We will also suppress console.error logs for re the console.error logs for redirect in the next framework. |
| } as Meta; | ||
|
|
||
| export const SingletonStateGetsInvalidatedAfterRedirecting: StoryObj = { | ||
| play: async ({ canvasElement, step }) => { |
There was a problem hiding this comment.
style: The step parameter is defined but never used in the play function
| {Object.entries(params).map(([key, value]) => ( | ||
| <li key={key}> | ||
| {key}: {value} | ||
| </li> |
There was a problem hiding this comment.
logic: Potential runtime error if value is an array - Object.entries(params) can return [string, string | string[]] but template expects string
kasperpeulen
changed the base branch from
next
to
kasper/nextjs-vite-rsc-base
|
View your CI Pipeline Execution ↗ for commit b836217
☁️ Nx Cloud last updated this comment at |
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 2 | 🚨 +2 🚨 |
| Self size | 0 B | 508 KB | 🚨 +508 KB 🚨 |
| Dependency size | 0 B | 2.80 MB | 🚨 +2.80 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/addon-docs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 18 | 🚨 +18 🚨 |
| Self size | 0 B | 2.02 MB | 🚨 +2.02 MB 🚨 |
| Dependency size | 0 B | 10.21 MB | 🚨 +10.21 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/addon-jest
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 2 | 🚨 +2 🚨 |
| Self size | 0 B | 47 KB | 🚨 +47 KB 🚨 |
| Dependency size | 0 B | 53 KB | 🚨 +53 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/addon-links
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 1 | 🚨 +1 🚨 |
| Self size | 0 B | 15 KB | 🚨 +15 KB 🚨 |
| Dependency size | 0 B | 5 KB | 🚨 +5 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/addon-onboarding
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 0 | 0 |
| Self size | 0 B | 333 KB | 🚨 +333 KB 🚨 |
| Dependency size | 0 B | 667 B | 🚨 +667 B 🚨 |
| Bundle Size Analyzer | Link | Link |
storybook-addon-pseudo-states
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 0 | 0 |
| Self size | 0 B | 23 KB | 🚨 +23 KB 🚨 |
| Dependency size | 0 B | 686 B | 🚨 +686 B 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/addon-themes
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 1 | 🚨 +1 🚨 |
| Self size | 0 B | 20 KB | 🚨 +20 KB 🚨 |
| Dependency size | 0 B | 28 KB | 🚨 +28 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/addon-vitest
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 6 | 🚨 +6 🚨 |
| Self size | 0 B | 501 KB | 🚨 +501 KB 🚨 |
| Dependency size | 0 B | 1.53 MB | 🚨 +1.53 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/builder-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 11 | 🚨 +11 🚨 |
| Self size | 0 B | 330 KB | 🚨 +330 KB 🚨 |
| Dependency size | 0 B | 1.30 MB | 🚨 +1.30 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/builder-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 187 | 🚨 +187 🚨 |
| Self size | 0 B | 68 KB | 🚨 +68 KB 🚨 |
| Dependency size | 0 B | 31.78 MB | 🚨 +31.78 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 43 | 🚨 +43 🚨 |
| Self size | 0 B | 30.76 MB | 🚨 +30.76 MB 🚨 |
| Dependency size | 0 B | 17.31 MB | 🚨 +17.31 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/angular
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 187 | 🚨 +187 🚨 |
| Self size | 0 B | 126 KB | 🚨 +126 KB 🚨 |
| Dependency size | 0 B | 29.94 MB | 🚨 +29.94 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/ember
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 191 | 🚨 +191 🚨 |
| Self size | 0 B | 17 KB | 🚨 +17 KB 🚨 |
| Dependency size | 0 B | 28.49 MB | 🚨 +28.49 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/html-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 14 | 🚨 +14 🚨 |
| Self size | 0 B | 23 KB | 🚨 +23 KB 🚨 |
| Dependency size | 0 B | 1.67 MB | 🚨 +1.67 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 532 | 🚨 +532 🚨 |
| Self size | 0 B | 939 KB | 🚨 +939 KB 🚨 |
| Dependency size | 0 B | 58.47 MB | 🚨 +58.47 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 124 | 🚨 +124 🚨 |
| Self size | 0 B | 4.08 MB | 🚨 +4.08 MB 🚨 |
| Dependency size | 0 B | 21.61 MB | 🚨 +21.61 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite-rsc
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 133 | 🚨 +133 🚨 |
| Self size | 0 B | 3.23 MB | 🚨 +3.23 MB 🚨 |
| Dependency size | 0 B | 23.78 MB | 🚨 +23.78 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/preact-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 14 | 🚨 +14 🚨 |
| Self size | 0 B | 14 KB | 🚨 +14 KB 🚨 |
| Dependency size | 0 B | 1.65 MB | 🚨 +1.65 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 157 | 🚨 +157 🚨 |
| Self size | 0 B | 31 KB | 🚨 +31 KB 🚨 |
| Dependency size | 0 B | 23.00 MB | 🚨 +23.00 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 114 | 🚨 +114 🚨 |
| Self size | 0 B | 37 KB | 🚨 +37 KB 🚨 |
| Dependency size | 0 B | 19.55 MB | 🚨 +19.55 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 272 | 🚨 +272 🚨 |
| Self size | 0 B | 25 KB | 🚨 +25 KB 🚨 |
| Dependency size | 0 B | 43.43 MB | 🚨 +43.43 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/server-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 199 | 🚨 +199 🚨 |
| Self size | 0 B | 17 KB | 🚨 +17 KB 🚨 |
| Dependency size | 0 B | 33.03 MB | 🚨 +33.03 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/svelte-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 19 | 🚨 +19 🚨 |
| Self size | 0 B | 59 KB | 🚨 +59 KB 🚨 |
| Dependency size | 0 B | 26.79 MB | 🚨 +26.79 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/sveltekit
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 20 | 🚨 +20 🚨 |
| Self size | 0 B | 58 KB | 🚨 +58 KB 🚨 |
| Dependency size | 0 B | 26.85 MB | 🚨 +26.85 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/vue3-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 109 | 🚨 +109 🚨 |
| Self size | 0 B | 38 KB | 🚨 +38 KB 🚨 |
| Dependency size | 0 B | 43.78 MB | 🚨 +43.78 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/web-components-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 15 | 🚨 +15 🚨 |
| Self size | 0 B | 20 KB | 🚨 +20 KB 🚨 |
| Dependency size | 0 B | 1.70 MB | 🚨 +1.70 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 187 | 🚨 +187 🚨 |
| Self size | 0 B | 917 KB | 🚨 +917 KB 🚨 |
| Dependency size | 0 B | 80.39 MB | 🚨 +80.39 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 169 | 🚨 +169 🚨 |
| Self size | 0 B | 35 KB | 🚨 +35 KB 🚨 |
| Dependency size | 0 B | 76.82 MB | 🚨 +76.82 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/core-webpack
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 1 | 🚨 +1 🚨 |
| Self size | 0 B | 12 KB | 🚨 +12 KB 🚨 |
| Dependency size | 0 B | 28 KB | 🚨 +28 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 44 | 🚨 +44 🚨 |
| Self size | 0 B | 1.54 MB | 🚨 +1.54 MB 🚨 |
| Dependency size | 0 B | 48.07 MB | 🚨 +48.07 MB 🚨 |
| Bundle Size Analyzer | node | node |
@storybook/csf-plugin
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 9 | 🚨 +9 🚨 |
| Self size | 0 B | 9 KB | 🚨 +9 KB 🚨 |
| Dependency size | 0 B | 1.27 MB | 🚨 +1.27 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
eslint-plugin-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 35 | 🚨 +35 🚨 |
| Self size | 0 B | 139 KB | 🚨 +139 KB 🚨 |
| Dependency size | 0 B | 3.15 MB | 🚨 +3.15 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-dom-shim
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 0 | 0 |
| Self size | 0 B | 22 KB | 🚨 +22 KB 🚨 |
| Dependency size | 0 B | 785 B | 🚨 +785 B 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/preset-create-react-app
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 68 | 🚨 +68 🚨 |
| Self size | 0 B | 36 KB | 🚨 +36 KB 🚨 |
| Dependency size | 0 B | 5.98 MB | 🚨 +5.98 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/preset-react-webpack
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 170 | 🚨 +170 🚨 |
| Self size | 0 B | 21 KB | 🚨 +21 KB 🚨 |
| Dependency size | 0 B | 30.93 MB | 🚨 +30.93 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/preset-server-webpack
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 10 | 🚨 +10 🚨 |
| Self size | 0 B | 8 KB | 🚨 +8 KB 🚨 |
| Dependency size | 0 B | 1.20 MB | 🚨 +1.20 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/html
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 2 | 🚨 +2 🚨 |
| Self size | 0 B | 30 KB | 🚨 +30 KB 🚨 |
| Dependency size | 0 B | 32 KB | 🚨 +32 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/preact
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 2 | 🚨 +2 🚨 |
| Self size | 0 B | 17 KB | 🚨 +17 KB 🚨 |
| Dependency size | 0 B | 32 KB | 🚨 +32 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 2 | 🚨 +2 🚨 |
| Self size | 0 B | 883 KB | 🚨 +883 KB 🚨 |
| Dependency size | 0 B | 28 KB | 🚨 +28 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/server
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 3 | 🚨 +3 🚨 |
| Self size | 0 B | 9 KB | 🚨 +9 KB 🚨 |
| Dependency size | 0 B | 716 KB | 🚨 +716 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/svelte
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 2 | 🚨 +2 🚨 |
| Self size | 0 B | 48 KB | 🚨 +48 KB 🚨 |
| Dependency size | 0 B | 230 KB | 🚨 +230 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/vue3
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 3 | 🚨 +3 🚨 |
| Self size | 0 B | 61 KB | 🚨 +61 KB 🚨 |
| Dependency size | 0 B | 211 KB | 🚨 +211 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/web-components
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 3 | 🚨 +3 🚨 |
| Self size | 0 B | 43 KB | 🚨 +43 KB 🚨 |
| Dependency size | 0 B | 47 KB | 🚨 +47 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a new RSC framework package Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CLI as Storybook CLI
participant BV as builder-vite/build.ts
participant V as vite (createBuilder)
CLI->>BV: build(options)
BV->>V: createBuilder(options, null)
V-->>BV: builder
BV->>V: builder.buildApp()
V-->>CLI: build artifacts
note right of V: Replaces direct viteBuild(...) with builder.buildApp()
sequenceDiagram
autonumber
participant Preview as Preview runtime
participant Loader as load-client-dev
participant Vite as Vite Dev Server
participant Server as RSC server handler
participant Client as react-client
Preview->>Loader: call loadClient()
Loader->>Vite: fetch /@vite/invoke-react-client {payload}
Vite->>Server: forward invocation
Server-->>Loader: stream/module payload
Loader->>Client: runner.import('.../react-client')
Client-->>Preview: mount / rerender / unmount via streamed updates
note over Loader,Server: browser-mode RSC virtual module & invoke endpoint flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
code/frameworks/nextjs-vite-rsc/template/stories/DynamicImport.stories.tsx (1)
19-21: Fix meta type to match exported component
meta.componentisComponent, but thesatisfiesclause targetsMeta<typeof DynamicComponent>, which forces a different component type and fails under strict typing. Adjust the meta type toComponentso the Storybook types compile cleanly.-} satisfies Meta<typeof DynamicComponent>; +} satisfies Meta<typeof Component>;code/frameworks/nextjs-vite-rsc/src/preview.tsx (1)
21-26: Make global patches HMR-safe; avoid duplicate meta and multiple error handlers.Hot reload will re-run this module, causing multiple
<meta>tags and stacked listeners.function addNextHeadCount() { - const meta = document.createElement('meta'); - meta.name = 'next-head-count'; - meta.content = '0'; - document.head.appendChild(meta); + if (!document.head.querySelector('meta[name="next-head-count"]')) { + const meta = document.createElement('meta'); + meta.name = 'next-head-count'; + meta.content = '0'; + document.head.appendChild(meta); + } } @@ -const origConsoleError = globalThis.console.error; -globalThis.console.error = (...args: unknown[]) => { - const error = args[0]; - if (isNextRouterError(error) || isAsyncClientComponentError(error)) { - return; - } - origConsoleError.apply(globalThis.console, args); -}; +if (!(globalThis as any).__sb_nextjs_rsc_console_error_patched__) { + const origConsoleError = globalThis.console.error; + globalThis.console.error = (...args: unknown[]) => { + const error = args[0]; + if (isNextRouterError(error) || isAsyncClientComponentError(error)) { + return; + } + origConsoleError.apply(globalThis.console, args); + }; + (globalThis as any).__sb_nextjs_rsc_console_error_patched__ = true; +} @@ -globalThis.addEventListener('error', (ev: WindowEventMap['error']): void => { - if (isNextRouterError(ev.error) || isAsyncClientComponentError(ev.error)) { - ev.preventDefault(); - return; - } -}); +if (!(globalThis as any).__sb_nextjs_rsc_error_handler__) { + globalThis.addEventListener('error', (ev: WindowEventMap['error']): void => { + if (isNextRouterError(ev.error) || isAsyncClientComponentError(ev.error)) { + ev.preventDefault(); + return; + } + }); + (globalThis as any).__sb_nextjs_rsc_error_handler__ = true; +}Also applies to: 38-47, 49-54
code/frameworks/nextjs-vite-rsc/template/stories/Redirect.stories.tsx (1)
30-30: Typo: “suspress” → “suppress”.- // We will also suspress console.error logs for re the console.error logs for redirect in the next framework. + // We will also suppress console.error logs for redirect in the next framework.code/frameworks/nextjs-vite-rsc/package.json (1)
107-111: Peer ranges allow React 16/17, which are incompatible with RSC.Tighten
react/react-domto 18+ to prevent unsupported installs.Apply:
- "react": "^16.8.0 || ^17.0.0 || ^18.0.0 || ^19.0.0", - "react-dom": "^16.8.0 || ^17.0.0 || ^18.0.0 || ^19.0.0", + "react": "^18.2.0 || ^19.0.0", + "react-dom": "^18.2.0 || ^19.0.0",
🧹 Nitpick comments (22)
scripts/build/utils/entry-utils.ts (1)
74-78: Don’t hard‑code framework‑specific virtual modules in a global build util; make it data‑driven.These Vite virtual IDs are specific to the RSC browser mode. Baking them into getExternal couples all packages to one framework and leaves a lingering TODO. Read extra externals from the package’s package.json instead.
Apply this diff:
- // TODO where to place this - 'virtual:vite-rsc-browser-mode/load-client', - 'virtual:vite-rsc-browser-mode/build-server-references', - 'virtual:vite-rsc-browser-mode/build-client-references' + // Allow packages to declare additional runtime externals (e.g., Vite virtual modules) + ...(packageJson.storybook?.runtimeExternals ?? [])Follow‑up: add
"storybook": { "runtimeExternals": ["virtual:vite-rsc-browser-mode/load-client","virtual:vite-rsc-browser-mode/build-server-references","virtual:vite-rsc-browser-mode/build-client-references"] }to the nextjs‑vite‑rsc package.json.code/frameworks/nextjs-vite-rsc/src/rsc/client.tsx (1)
127-129: Consider rendering cache.rsc inside RedirectBoundary behind a flag.Commenting it out may hide RSC regressions. Gate it with a prop to toggle between children and cache.rsc during testing.
code/frameworks/nextjs-vite-rsc/src/routing/page-router-provider.tsx (1)
13-15: Consider marking this provider as a client component.Adding
"use client"at the top avoids accidental server evaluation under RSC setups and clarifies intent. Optional but safer for router context providers.code/frameworks/nextjs-vite-rsc/src/load-client-dev.tsx (1)
8-16: Ensure fetch transport surfaces server errorsRight now any non-2xx response will bubble up as a generic JSON parse failure, which hides the actual server-side error message. Please surface the HTTP status and body so devs can see what went wrong.
const response = await fetch( '/@vite/invoke-react-client?' + new URLSearchParams({ data: JSON.stringify(payload), }) ); - return response.json(); + if (!response.ok) { + const errorText = await response.text().catch(() => response.statusText); + throw new Error( + `Failed to invoke react client (${response.status} ${response.statusText}): ${errorText}` + ); + } + return response.json();code/frameworks/nextjs-vite-rsc/src/rsc/flight-router-state.test.ts (2)
5-6: Tighten the test name.Rename to clearly state behavior, e.g., “builds flight router state for dynamic segments with query”.
-test('parse route and url to route true', () => { +test('builds flight router state for /note/[id]/[slug] with ?a=1', () => {
5-46: Add root and mismatch tests & fix root fullPath handling
- Add tests in
flight-router-state.test.ts:test('root route builds __PAGE__ with query', () => { expect(buildFlightRouterState('/', '/', '?q=1')).toMatchInlineSnapshot(); }); test('throws on pattern/path mismatch', () => { expect(() => buildFlightRouterState('/a/[b]', '/a', '')).toThrowError(); });- In
flight-router-state.ts, adjust the root‐wrapper to drop the query fromfullPath:- const childState = - patternSegs.length === 0 - ? makePageState('/' + (search.startsWith('?') ? search.slice(1) : search)) - : descend(0, ''); + const childState = + patternSegs.length === 0 + ? makePageState('/' + (search.startsWith('?') ? '' : search)) + : descend(0, '');code/lib/cli-storybook/src/sandbox-templates.ts (1)
252-274: Disambiguate template name and double‑check renderer mapping.
- Name duplicates the non‑RSC template; add “, RSC” for clarity in dashboards.
- Verify
expected.renderer. Core usually maps Next/Vite frameworks to@storybook/react; ensure this matches how the new framework is registered, or adjust here. Mismatch will fail the template assertion.Proposed tweak:
- 'nextjs-vite-rsc/default-ts': { - name: 'Next.js Latest (Vite | TypeScript)', + 'nextjs-vite-rsc/default-ts': { + name: 'Next.js Latest (Vite | TypeScript, RSC)', @@ - expected: { - framework: '@storybook/nextjs-vite-rsc', - renderer: '@storybook/nextjs-vite-rsc', - builder: '@storybook/builder-vite', - }, + expected: { + framework: '@storybook/nextjs-vite-rsc', + renderer: '@storybook/react', // if core maps RSC to react renderer + builder: '@storybook/builder-vite', + },If the RSC package truly acts as a renderer, align core mapping accordingly instead. Please confirm with the mapping in
framework-to-renderer.ts.Optional: add a TODO next to
inDevelopment: trueto track removal before GA.code/builders/builder-vite/src/build.ts (1)
18-18: Guard for older Vite versions and avoid passing null to createBuilder.Vite 7 exports
createBuilder()and aViteBuilder.buildApp()entrypoint; add a fallback toviteBuild()for older Vite to keep backward compatibility, and passundefinedinstead ofnullfor options. (jsdocs.io)- const { build: viteBuild, mergeConfig, createBuilder } = await import('vite'); + const { build: viteBuild, mergeConfig, createBuilder } = await import('vite'); @@ - await (await createBuilder(await sanitizeEnvVars(options, finalConfig), null)).buildApp(); + const sanitized = await sanitizeEnvVars(options, finalConfig); + if (typeof createBuilder === 'function') { + const builder = await createBuilder(sanitized, undefined); + await builder.buildApp(); + } else { + // Fallback for older Vite without the builder API + await viteBuild(sanitized as any); + }code/lib/create-storybook/src/bin/modernInputs.ts (1)
3-3: De-duplicate framework sources of truth.This TODO keeps biting us—recommend importing this list from core (or generating it once) to prevent drift between CLI and core helpers.
code/frameworks/nextjs-vite-rsc/src/rsc/flight-router-state.ts (1)
15-20: Strict pattern matching rejects catch‑all/optional segments.If you plan to support
[...slug]/[[...slug]], the strict length equality will misbehave. Consider extending parsing or documenting the limitation.code/frameworks/nextjs-vite-rsc/package.json (1)
118-120: Declare supported Node engines (Vite 7/Next 15 expect modern Node).Recommend Node >= 18.17 (or >=20 if that’s your project policy) to match Vite/Next requirements.
Apply:
"publishConfig": { "access": "public" - }, + }, + "engines": { + "node": ">=18.17.0" + },code/frameworks/nextjs-vite-rsc/src/react-client.tsx (2)
42-51: Guard server callback updates until setter is readyEven with the above fix, defensive code helps: if a callback races very early,
setPayloadcan still be undefined. Add a guard or queue.- setPayload(payload); + if (setPayload) { + setPayload(payload); + }
69-76: Reset global server callback on unmount to avoid test leakage
setServerCallbackis global; without cleanup, later tests may target a disposed root.function unmount() { + // prevent late server callbacks from targeting an unmounted root + ReactClient.setServerCallback(async () => {}); reactRoot.unmount(); }code/frameworks/nextjs-vite-rsc/src/virtual.d.ts (2)
1-14: Tighten types: prefer unknown over any for dynamic importsAvoid
anyin ambient declarations;unknownpreserves type safety. As per coding guidelines.declare module 'virtual:vite-rsc-browser-mode/build-client-references' { - const default_: Record<string, () => Promise<any>>; + const default_: Record<string, () => Promise<unknown>>; export default default_; } declare module 'virtual:vite-rsc-browser-mode/build-server-references' { - const default_: Record<string, () => Promise<any>>; + const default_: Record<string, () => Promise<unknown>>; export default default_; }
18-24: Optional: declare global augmentation explicitlyTo reduce chances of duplicate ambient merges in complex monorepos, consider wrapping
ImportMetaEnv/ImportMetaindeclare global { ... }. Current approach works with Vite’s client types; this is a nit.code/frameworks/nextjs-vite-rsc/src/rsc-plugin.ts (3)
140-141: Double client build call — intentional?
await builder.build(reactServer);is invoked twice back-to-back before switching modes. If this is required for scanning/reference collection, add a comment; otherwise, remove the duplicate to cut build time.
46-50: noExternal: true in react_client may increase bundle sizeBundling all deps can slow builds and hinder caching. If not strictly needed, consider narrowing
noExternalinstead of blankettrue.
112-121: Dev middleware endpoint is wide open
/@vite/invoke-react-clientaccepts arbitrarydataand forwards tohandleInvoke. Since this is dev-only, it's acceptable, but consider guarding with method check and basic validation to reduce footguns.code/frameworks/nextjs-vite-rsc/src/preset.ts (2)
62-75: Flatten plugin arrays for clarity.
rscBrowserModePlugin()andvitestPluginNext()return arrays. Vite accepts nested PluginOptions, but flattening improves readability and avoids edge cases with custom merging.- plugins: [...(reactConfig?.plugins ?? []), rscBrowserModePlugin(), vitestPluginNext()], + plugins: [ + ...(reactConfig?.plugins ?? []), + ...rscBrowserModePlugin(), + ...vitestPluginNext(), + ],
49-53: Use the final root for PostCSS search path.You call
postCssLoadConfigusingconfig.root. IfreactViteFinaladjustsroot, preferreactConfig.rootto avoid false negatives.- const searchPath = typeof inlineOptions === 'string' ? inlineOptions : config.root; + const searchPath = + typeof inlineOptions === 'string' ? inlineOptions : (reactConfig as any)?.root ?? config.root;code/frameworks/nextjs-vite-rsc/src/testing-library.tsx (2)
72-74: Guard against stale registry reads.Avoid
!on the lookup; throw with context if missing.- } else { - root = mountedRootEntries.find((it) => it.container === container)!.root; - } + } else { + const entry = mountedRootEntries.find((it) => it.container === container); + if (!entry) throw new Error('Invariant: mounted root not found for container'); + root = entry.root; + }
27-28: Unused optionrerenderOnServerAction.Either implement the behavior (auto‑rerender after server actions) or remove the option until needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
code/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (47)
code/builders/builder-vite/src/build.ts(2 hunks)code/core/src/cli/helpers.ts(1 hunks)code/core/src/common/utils/framework-to-renderer.ts(1 hunks)code/core/src/common/utils/get-storybook-info.ts(1 hunks)code/core/src/types/modules/frameworks.ts(1 hunks)code/frameworks/nextjs-vite-rsc/build-config.ts(1 hunks)code/frameworks/nextjs-vite-rsc/package.json(4 hunks)code/frameworks/nextjs-vite-rsc/project.json(1 hunks)code/frameworks/nextjs-vite-rsc/src/export-mocks/headers/cookies.ts(1 hunks)code/frameworks/nextjs-vite-rsc/src/index.ts(1 hunks)code/frameworks/nextjs-vite-rsc/src/load-client-dev.tsx(1 hunks)code/frameworks/nextjs-vite-rsc/src/portable-stories.ts(3 hunks)code/frameworks/nextjs-vite-rsc/src/preset.ts(3 hunks)code/frameworks/nextjs-vite-rsc/src/preview.tsx(3 hunks)code/frameworks/nextjs-vite-rsc/src/react-client.tsx(1 hunks)code/frameworks/nextjs-vite-rsc/src/routing/app-router-provider.tsx(1 hunks)code/frameworks/nextjs-vite-rsc/src/routing/page-router-provider.tsx(1 hunks)code/frameworks/nextjs-vite-rsc/src/rsc-plugin.ts(1 hunks)code/frameworks/nextjs-vite-rsc/src/rsc/client.tsx(1 hunks)code/frameworks/nextjs-vite-rsc/src/rsc/flight-router-state.test.ts(1 hunks)code/frameworks/nextjs-vite-rsc/src/rsc/flight-router-state.ts(1 hunks)code/frameworks/nextjs-vite-rsc/src/testing-library.tsx(1 hunks)code/frameworks/nextjs-vite-rsc/src/types.ts(1 hunks)code/frameworks/nextjs-vite-rsc/src/virtual.d.ts(1 hunks)code/frameworks/nextjs-vite-rsc/template/cli/ts/Button.stories.ts(1 hunks)code/frameworks/nextjs-vite-rsc/template/cli/ts/Header.stories.ts(1 hunks)code/frameworks/nextjs-vite-rsc/template/cli/ts/Page.stories.ts(1 hunks)code/frameworks/nextjs-vite-rsc/template/stories/DynamicImport.stories.tsx(1 hunks)code/frameworks/nextjs-vite-rsc/template/stories/Font.stories.tsx(1 hunks)code/frameworks/nextjs-vite-rsc/template/stories/GetImageProps.stories.tsx(1 hunks)code/frameworks/nextjs-vite-rsc/template/stories/Head.stories.tsx(1 hunks)code/frameworks/nextjs-vite-rsc/template/stories/Image.stories.tsx(1 hunks)code/frameworks/nextjs-vite-rsc/template/stories/ImageLegacy.stories.tsx(1 hunks)code/frameworks/nextjs-vite-rsc/template/stories/Link.stories.tsx(1 hunks)code/frameworks/nextjs-vite-rsc/template/stories/Navigation.stories.tsx(1 hunks)code/frameworks/nextjs-vite-rsc/template/stories/NextHeader.stories.tsx(1 hunks)code/frameworks/nextjs-vite-rsc/template/stories/RSC.stories.tsx(1 hunks)code/frameworks/nextjs-vite-rsc/template/stories/Redirect.stories.tsx(1 hunks)code/frameworks/nextjs-vite-rsc/template/stories/Router.stories.tsx(1 hunks)code/frameworks/nextjs-vite-rsc/template/stories/ServerActions.stories.tsx(1 hunks)code/frameworks/nextjs-vite-rsc/template/stories/StyledJsx.stories.tsx(1 hunks)code/lib/cli-storybook/src/sandbox-templates.ts(1 hunks)code/lib/create-storybook/src/bin/modernInputs.ts(3 hunks)code/lib/create-storybook/src/generators/baseGenerator.ts(1 hunks)code/lib/create-storybook/src/ink/steps/checks/frameworkTest.tsx(1 hunks)scripts/build/entry-configs.ts(2 hunks)scripts/build/utils/entry-utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/frameworks/nextjs-vite-rsc/src/index.tscode/frameworks/nextjs-vite-rsc/template/cli/ts/Header.stories.tscode/frameworks/nextjs-vite-rsc/src/rsc/flight-router-state.tscode/lib/create-storybook/src/bin/modernInputs.tscode/lib/create-storybook/src/generators/baseGenerator.tscode/frameworks/nextjs-vite-rsc/src/virtual.d.tscode/frameworks/nextjs-vite-rsc/template/cli/ts/Page.stories.tscode/lib/cli-storybook/src/sandbox-templates.tscode/frameworks/nextjs-vite-rsc/template/stories/Redirect.stories.tsxcode/frameworks/nextjs-vite-rsc/template/stories/Head.stories.tsxcode/core/src/types/modules/frameworks.tscode/core/src/cli/helpers.tscode/frameworks/nextjs-vite-rsc/template/stories/ImageLegacy.stories.tsxcode/core/src/common/utils/get-storybook-info.tscode/frameworks/nextjs-vite-rsc/template/stories/StyledJsx.stories.tsxcode/frameworks/nextjs-vite-rsc/src/types.tscode/lib/create-storybook/src/ink/steps/checks/frameworkTest.tsxcode/frameworks/nextjs-vite-rsc/build-config.tscode/frameworks/nextjs-vite-rsc/src/rsc/client.tsxcode/frameworks/nextjs-vite-rsc/src/react-client.tsxcode/frameworks/nextjs-vite-rsc/template/stories/RSC.stories.tsxcode/core/src/common/utils/framework-to-renderer.tscode/frameworks/nextjs-vite-rsc/src/load-client-dev.tsxcode/frameworks/nextjs-vite-rsc/template/stories/Font.stories.tsxcode/frameworks/nextjs-vite-rsc/src/routing/app-router-provider.tsxcode/frameworks/nextjs-vite-rsc/template/stories/Image.stories.tsxscripts/build/entry-configs.tscode/frameworks/nextjs-vite-rsc/template/stories/Router.stories.tsxcode/frameworks/nextjs-vite-rsc/template/stories/ServerActions.stories.tsxcode/frameworks/nextjs-vite-rsc/template/stories/Link.stories.tsxcode/frameworks/nextjs-vite-rsc/template/stories/Navigation.stories.tsxcode/frameworks/nextjs-vite-rsc/template/stories/DynamicImport.stories.tsxcode/builders/builder-vite/src/build.tscode/frameworks/nextjs-vite-rsc/src/rsc/flight-router-state.test.tscode/frameworks/nextjs-vite-rsc/template/cli/ts/Button.stories.tscode/frameworks/nextjs-vite-rsc/src/export-mocks/headers/cookies.tscode/frameworks/nextjs-vite-rsc/template/stories/NextHeader.stories.tsxcode/frameworks/nextjs-vite-rsc/src/portable-stories.tscode/frameworks/nextjs-vite-rsc/src/rsc-plugin.tscode/frameworks/nextjs-vite-rsc/src/preview.tsxcode/frameworks/nextjs-vite-rsc/src/testing-library.tsxscripts/build/utils/entry-utils.tscode/frameworks/nextjs-vite-rsc/src/routing/page-router-provider.tsxcode/frameworks/nextjs-vite-rsc/template/stories/GetImageProps.stories.tsxcode/frameworks/nextjs-vite-rsc/src/preset.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/frameworks/nextjs-vite-rsc/src/index.tscode/frameworks/nextjs-vite-rsc/template/cli/ts/Header.stories.tscode/frameworks/nextjs-vite-rsc/src/rsc/flight-router-state.tscode/lib/create-storybook/src/bin/modernInputs.tscode/lib/create-storybook/src/generators/baseGenerator.tscode/frameworks/nextjs-vite-rsc/src/virtual.d.tscode/frameworks/nextjs-vite-rsc/template/cli/ts/Page.stories.tscode/lib/cli-storybook/src/sandbox-templates.tscode/frameworks/nextjs-vite-rsc/template/stories/Redirect.stories.tsxcode/frameworks/nextjs-vite-rsc/template/stories/Head.stories.tsxcode/core/src/types/modules/frameworks.tscode/core/src/cli/helpers.tscode/frameworks/nextjs-vite-rsc/template/stories/ImageLegacy.stories.tsxcode/core/src/common/utils/get-storybook-info.tscode/frameworks/nextjs-vite-rsc/template/stories/StyledJsx.stories.tsxcode/frameworks/nextjs-vite-rsc/src/types.tscode/lib/create-storybook/src/ink/steps/checks/frameworkTest.tsxcode/frameworks/nextjs-vite-rsc/build-config.tscode/frameworks/nextjs-vite-rsc/src/rsc/client.tsxcode/frameworks/nextjs-vite-rsc/src/react-client.tsxcode/frameworks/nextjs-vite-rsc/template/stories/RSC.stories.tsxcode/core/src/common/utils/framework-to-renderer.tscode/frameworks/nextjs-vite-rsc/src/load-client-dev.tsxcode/frameworks/nextjs-vite-rsc/template/stories/Font.stories.tsxcode/frameworks/nextjs-vite-rsc/src/routing/app-router-provider.tsxcode/frameworks/nextjs-vite-rsc/template/stories/Image.stories.tsxscripts/build/entry-configs.tscode/frameworks/nextjs-vite-rsc/template/stories/Router.stories.tsxcode/frameworks/nextjs-vite-rsc/template/stories/ServerActions.stories.tsxcode/frameworks/nextjs-vite-rsc/template/stories/Link.stories.tsxcode/frameworks/nextjs-vite-rsc/template/stories/Navigation.stories.tsxcode/frameworks/nextjs-vite-rsc/template/stories/DynamicImport.stories.tsxcode/builders/builder-vite/src/build.tscode/frameworks/nextjs-vite-rsc/src/rsc/flight-router-state.test.tscode/frameworks/nextjs-vite-rsc/template/cli/ts/Button.stories.tscode/frameworks/nextjs-vite-rsc/src/export-mocks/headers/cookies.tscode/frameworks/nextjs-vite-rsc/template/stories/NextHeader.stories.tsxcode/frameworks/nextjs-vite-rsc/src/portable-stories.tscode/frameworks/nextjs-vite-rsc/src/rsc-plugin.tscode/frameworks/nextjs-vite-rsc/src/preview.tsxcode/frameworks/nextjs-vite-rsc/src/testing-library.tsxscripts/build/utils/entry-utils.tscode/frameworks/nextjs-vite-rsc/src/routing/page-router-provider.tsxcode/frameworks/nextjs-vite-rsc/template/stories/GetImageProps.stories.tsxcode/frameworks/nextjs-vite-rsc/src/preset.ts
code/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
code/**/*.{test,spec}.{ts,tsx}: Place all test files under the code/ directory
Name test files as *.test.ts, *.test.tsx, *.spec.ts, or *.spec.tsx
Files:
code/frameworks/nextjs-vite-rsc/src/rsc/flight-router-state.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Use vi.mocked() to type and access mocked functions
Implement mock behaviors in beforeEach blocks
Mock all required dependencies that the test subject uses
Mock implementations should be placed in beforeEach blocks
Each mock implementation should return a Promise for async functions
Mock implementations should match the expected return type of the original function
Use vi.mocked() to access and implement mock behaviors
Mock all required properties and methods that the test subject uses
Avoid direct function mocking without vi.mocked()
Avoid mock implementations outside of beforeEach blocks
Avoid mocking without the spy: true option
Avoid inline mock implementations within test cases
Avoid mocking only a subset of required dependencies
Mock at the highest level of abstraction needed
Keep mock implementations simple and focused
Use type-safe mocking with vi.mocked()
Document complex mock behaviors
Group related mocks together
Files:
code/frameworks/nextjs-vite-rsc/src/rsc/flight-router-state.test.ts
🧠 Learnings (13)
📚 Learning: 2025-09-25T09:21:27.274Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-25T09:21:27.274Z
Learning: Applies to test-storybooks/** : Maintain test configurations and assets under test-storybooks/ for Storybook testing
Applied to files:
code/frameworks/nextjs-vite-rsc/template/cli/ts/Header.stories.tscode/frameworks/nextjs-vite-rsc/template/cli/ts/Page.stories.tscode/core/src/common/utils/get-storybook-info.tscode/frameworks/nextjs-vite-rsc/template/stories/RSC.stories.tsxcode/frameworks/nextjs-vite-rsc/template/stories/Font.stories.tsxcode/frameworks/nextjs-vite-rsc/template/stories/Image.stories.tsxcode/frameworks/nextjs-vite-rsc/template/stories/ServerActions.stories.tsxcode/frameworks/nextjs-vite-rsc/template/cli/ts/Button.stories.tscode/frameworks/nextjs-vite-rsc/src/preset.ts
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Group related mocks together
Applied to files:
code/frameworks/nextjs-vite-rsc/build-config.tscode/frameworks/nextjs-vite-rsc/package.json
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies
Applied to files:
code/frameworks/nextjs-vite-rsc/build-config.tscode/frameworks/nextjs-vite-rsc/package.json
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required dependencies that the test subject uses
Applied to files:
code/frameworks/nextjs-vite-rsc/build-config.ts
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors
Applied to files:
code/frameworks/nextjs-vite-rsc/build-config.ts
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Keep mock implementations simple and focused
Applied to files:
code/frameworks/nextjs-vite-rsc/build-config.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses
Applied to files:
code/frameworks/nextjs-vite-rsc/build-config.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases
Applied to files:
code/frameworks/nextjs-vite-rsc/build-config.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock at the highest level of abstraction needed
Applied to files:
code/frameworks/nextjs-vite-rsc/build-config.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in beforeEach blocks
Applied to files:
code/frameworks/nextjs-vite-rsc/build-config.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vi.mocked() to access and implement mock behaviors
Applied to files:
code/frameworks/nextjs-vite-rsc/build-config.ts
📚 Learning: 2025-09-24T09:39:39.209Z
Learnt from: ndelangen
PR: storybookjs/storybook#32507
File: code/core/src/manager/globals/globals-module-info.ts:25-33
Timestamp: 2025-09-24T09:39:39.209Z
Learning: In Storybook, storybook/actions/decorator is a preview-only entrypoint and should not be included in manager globals configuration. The duplicatedKeys array in code/core/src/manager/globals/globals-module-info.ts is specifically for manager-side externalization, not preview entrypoints.
Applied to files:
code/frameworks/nextjs-vite-rsc/src/preview.tsxcode/frameworks/nextjs-vite-rsc/src/preset.ts
📚 Learning: 2025-09-17T07:31:04.432Z
Learnt from: ndelangen
PR: storybookjs/storybook#32484
File: code/core/package.json:326-326
Timestamp: 2025-09-17T07:31:04.432Z
Learning: In Storybook's core package, dependencies like `open` are bundled into the final distribution during the build process, so they should remain in devDependencies rather than being moved to dependencies. End users don't need these packages as separate runtime dependencies since they're included in the bundled code.
Applied to files:
code/frameworks/nextjs-vite-rsc/src/preset.ts
🧬 Code graph analysis (7)
code/frameworks/nextjs-vite-rsc/src/types.ts (1)
code/core/src/types/modules/core-common.ts (1)
CompatibleString(658-658)
code/frameworks/nextjs-vite-rsc/src/rsc/client.tsx (1)
code/frameworks/nextjs-vite-rsc/src/rsc/flight-router-state.ts (1)
buildFlightRouterState(6-64)
code/frameworks/nextjs-vite-rsc/src/react-client.tsx (1)
code/frameworks/nextjs-vite-rsc/src/testing-library.tsx (2)
RenderConfiguration(101-104)initialize(115-133)
code/frameworks/nextjs-vite-rsc/src/rsc/flight-router-state.test.ts (1)
code/frameworks/nextjs-vite-rsc/src/rsc/flight-router-state.ts (1)
buildFlightRouterState(6-64)
code/frameworks/nextjs-vite-rsc/src/preview.tsx (4)
code/frameworks/nextjs-vite-rsc/src/rsc/client.tsx (1)
NextRouter(38-79)code/renderers/react/src/types.ts (2)
ReactRenderer(8-12)RenderContext(6-6)code/frameworks/nextjs-vite-rsc/src/react-client.tsx (1)
initialize(95-109)code/frameworks/nextjs-vite-rsc/src/testing-library.tsx (2)
initialize(115-133)renderServer(17-88)
code/frameworks/nextjs-vite-rsc/src/testing-library.tsx (2)
code/frameworks/nextjs-vite-rsc/src/react-client.tsx (4)
TestingLibraryClientRoot(14-14)FetchRsc(16-19)RscPayload(9-12)initialize(95-109)code/frameworks/nextjs-vite-rsc/src/load-client-dev.tsx (1)
loadClient(3-25)
code/frameworks/nextjs-vite-rsc/src/preset.ts (1)
code/frameworks/nextjs-vite-rsc/src/rsc-plugin.ts (2)
rscBrowserModePlugin(4-209)vitestPluginNext(211-260)
⏰ 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: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (36)
code/frameworks/nextjs-vite-rsc/template/cli/ts/Header.stories.ts (1)
1-1: Import path matches the new framework surface.Good catch switching the Storybook generics to
@storybook/nextjs-vite-rsc; this keeps the template aligned with the RSC framework package and preserves the existing TS typing flow.code/frameworks/nextjs-vite-rsc/template/cli/ts/Button.stories.ts (1)
1-1: Updated import is consistent with the RSC variant.The Meta/StoryObj types now come from the correct
@storybook/nextjs-vite-rscpackage, ensuring the CLI template stays in sync with the new framework.code/frameworks/nextjs-vite-rsc/template/stories/Link.stories.tsx (1)
3-3: RSC package import looks correct.Bringing the Storybook types in via
@storybook/nextjs-vite-rscmatches the framework rename and keeps the story typing intact.code/frameworks/nextjs-vite-rsc/template/stories/Font.stories.tsx (1)
1-1: Framework-specific import verified.
Meta/StoryObjnow resolve from@storybook/nextjs-vite-rsc, so this template will type-check against the new package without any extra tweaks.code/frameworks/nextjs-vite-rsc/template/stories/Image.stories.tsx (1)
3-3: Import realignment to RSC package is spot on.Switching the type import to
@storybook/nextjs-vite-rsckeeps the image stories aligned with the new Next.js/Vite RSC framework entry.code/frameworks/nextjs-vite-rsc/template/stories/ServerActions.stories.tsx (3)
45-49: Fix typo and noisy comment.Use “suppress” and simplify the sentence.
Apply this diff:
- // We will also suspress console.error logs for re the console.error logs for redirect in the next framework. + // We will also suppress console.error logs for redirects in the Next framework.
3-6: Verify root exports for Meta/StoryObj — package.json confirms that./cache.mock,./headers.mockand./navigation.mockare exported; please ensure the package’s main entrypoint also exports theMetaandStoryObjtypes to prevent template breakage.
76-80: Use play or loaders instead of unsupported beforeEach on StoryObj
TheStory(andStoryObj) type does not include abeforeEachhook—onlyplay,decorators,loaders, and params are supported. Move your setup into aloaderor inline inplayto keep types strict.code/frameworks/nextjs-vite-rsc/project.json (1)
2-2: LGTM: name aligns with directory and avoids collisions.code/frameworks/nextjs-vite-rsc/src/portable-stories.ts (1)
37-41: Docs updated to @storybook/nextjs-vite-rsc — good; ensure top‑level exports exist.The examples now point to the new package. Double‑check the package’s export map includes setProjectAnnotations/composeStory/composeStories at the root to prevent surprises for users.
Also applies to: 71-75, 115-118
code/frameworks/nextjs-vite-rsc/template/stories/Redirect.stories.tsx (1)
53-56: Remove unusedstepparam inplay.-export const SingletonStateGetsInvalidatedAfterRedirecting: StoryObj = { - play: async ({ canvasElement, step }) => { +export const SingletonStateGetsInvalidatedAfterRedirecting: StoryObj = { + play: async ({ canvasElement }) => { const canvas = within(canvasElement); await userEvent.click(canvas.getByRole('button')); }, };code/lib/create-storybook/src/generators/baseGenerator.ts (1)
202-209: LGTM: framework templates updated to includenextjs-vite-rsc.code/frameworks/nextjs-vite-rsc/template/stories/GetImageProps.stories.tsx (1)
3-3: Import path aligned with the new framework variantSwitching the Meta/StoryObj types to
@storybook/nextjs-vite-rsckeeps this template consistent with the RSC-aware framework package. Nice cleanup.code/frameworks/nextjs-vite-rsc/template/stories/NextHeader.stories.tsx (1)
1-3: Correct mocks/types source for RSC storiesPointing both the story types and the header/cookie mocks at the RSC package ensures the story exercises the right singleton state for the new framework.
code/core/src/types/modules/frameworks.ts (1)
8-8: Supported frameworks list updatedAdding
'nextjs-vite-rsc'here keeps the public typings in sync with the new framework entry.code/frameworks/nextjs-vite-rsc/src/routing/app-router-provider.tsx (1)
8-8: Router mock import matches the RSC bundleUpdating
getRouterto pull from the RSC mock package maintains the singleton behavior when consumers use the new framework entrypoint.code/core/src/common/utils/framework-to-renderer.ts (1)
14-14: Renderer mapping covers the new frameworkMapping
'nextjs-vite-rsc'to the React renderer keeps framework detection consistent across tooling.code/frameworks/nextjs-vite-rsc/template/stories/Router.stories.tsx (2)
3-4: Imports correctly switched to RSC variant.Type and mock imports point to @storybook/nextjs-vite-rsc; looks consistent with the new framework.
85-96: Fix duplicate/incorrect step assertions; first step should validate useRouter (DOM), not the Router singleton.Both steps assert
Router.*. Make the first step assert via the rendered DOM (which is driven byuseRouter()in Component) to match its title and avoid redundancy.- await step('Router property overrides should be available in useRouter fn', async () => { - await expect(Router.pathname).toBe('/hello'); - await expect(Router.query).toEqual({ foo: 'bar' }); - }); + await step('Router property overrides should be available in useRouter fn', async () => { + await expect(await canvas.findByText('pathname: /hello')).toBeInTheDocument(); + await expect(await canvas.findByText('foo: bar')).toBeInTheDocument(); + });If
toBeInTheDocumentisn’t available in your test environment, replace with a truthiness check on the returned elements.code/frameworks/nextjs-vite-rsc/template/stories/RSC.stories.tsx (2)
4-4: RSC typings source aligned.Switch to
@storybook/nextjs-vite-rscforMeta/StoryObjis correct and consistent with the new framework.
22-28: Confirm authoritative RSC parameter and override behavior.Found react.rsc: true in code/renderers/react/src/entry-preview-rsc.tsx and in story templates, while this story sets nextjs.rsc: false; the repository search did not reveal the parameter-merge/reader logic that determines precedence — confirm which key is authoritative and that story-level nextjs.rsc correctly overrides react.rsc.
Locations:
- code/renderers/react/src/entry-preview-rsc.tsx — parameters.react.rsc: true
- code/frameworks/nextjs-vite-rsc/template/stories/RSC.stories.tsx — story parameters.nextjs.rsc: false
code/frameworks/nextjs-vite-rsc/src/routing/page-router-provider.tsx (1)
9-9: Import path updated to RSC mock — looks good.Keeps the mock singleton semantics by importing from the package entry.
code/core/src/cli/helpers.ts (1)
151-152: Mapping the new framework to the Vite builder makes sense.Using
CoreBuilder.Vitefornextjs-vite-rscmatches the expectations set by the rest of the framework wiring.code/core/src/common/utils/get-storybook-info.ts (1)
43-44: Good to see the framework package registered.Adding
'@storybook/nextjs-vite-rsc'ensures detection flows resolve the new framework correctly.scripts/build/entry-configs.ts (1)
32-113: Build entry wiring looks correct.Importing the new build config and exposing it through
buildEntrieskeeps the package buildable alongside the existing Next.js variants.code/frameworks/nextjs-vite-rsc/build-config.ts (1)
31-41: New browser entry points are aligned with the exported surface.Registering
load-client-dev,react-client, andrsc/clienthere will let the builder emit the corresponding bundles.code/frameworks/nextjs-vite-rsc/template/stories/Navigation.stories.tsx (1)
3-4: Template imports now target the correct package.Switching to
@storybook/nextjs-vite-rsckeeps the scaffolded stories in sync with the new framework.code/lib/create-storybook/src/ink/steps/checks/frameworkTest.tsx (1)
17-19:nextjs-vite-rscsupport fully wired. Verified inclusion inFrameworktype andsupportedFrameworksNamesmapping.code/frameworks/nextjs-vite-rsc/template/stories/Head.stories.tsx (1)
3-4: Approve import change:MetaandStoryObjare re-exported from@storybook/reactin@storybook/nextjs-vite-rscviaexport * from '@storybook/react', so these imports are valid.code/frameworks/nextjs-vite-rsc/package.json (4)
2-2: Package name fix looks good.
14-14: Homepage path points to the wrong directory.Apply:
- "homepage": "https://github.com/storybookjs/storybook/tree/next/code/frameworks/nextjs-vite", + "homepage": "https://github.com/storybookjs/storybook/tree/next/code/frameworks/nextjs-vite-rsc",
21-22: Repository.directory is incorrect.Apply:
- "directory": "code/frameworks/nextjs" + "directory": "code/frameworks/nextjs-vite-rsc"
96-96: Verified compatibility for @vitejs/plugin-rsc@^0.4.31: supports Vite 7+ (requires Node ^20.19.0 or ≥22.12.0); Next.js 15 (React 19) isn’t officially supported out-of-the-box—custom integration per project is required.code/lib/create-storybook/src/bin/modernInputs.ts (1)
10-10: Approve nextjs-vite-rsc integration
Wiring is complete and type-safe—nextjs-vite-rscappears in all ID lists, package mappings, display names, core types, and build configs.code/frameworks/nextjs-vite-rsc/src/preset.ts (2)
30-35: Correct preview entrypoint for RSC.Appending
@storybook/nextjs-vite-rsc/previewlooks right and addresses earlier mis-reference to the non‑RSC preview.
26-27: Verify framework-to-renderer fallback is correct
nextjs-vite-rscis mapped to"react"inframework-to-renderer(core/src/common/utils/framework-to-renderer.ts:14); leavingrendererundefined in the preset will correctly fall back to React at runtime.
| export const optimizeViteDeps = [ | ||
| '@storybook/nextjs-vite/navigation.mock', | ||
| '@storybook/nextjs-vite/router.mock', | ||
| '@storybook/nextjs-vite > styled-jsx', | ||
| '@storybook/nextjs-vite > styled-jsx/style', | ||
| 'next/dist/client/components/is-next-router-error', | ||
| '@storybook/nextjs-vite-rsc/navigation.mock', | ||
| '@storybook/nextjs-vite-rsc/router.mock', | ||
| '@storybook/nextjs-vite-rsc > styled-jsx', | ||
| '@storybook/nextjs-vite-rsc > styled-jsx/style', | ||
| ]; |
There was a problem hiding this comment.
🧩 Analysis chain
Double‑check optimizeDeps include syntax for transitive deps.
Entries like @storybook/nextjs-vite-rsc > styled-jsx/... > styled-jsx/style are unconventional for Vite’s optimizeDeps.include. Ensure the builder/preset pipeline consumes this list and expands the dep > subdep form as intended (or switch to plain bare imports if not needed). Note that vitestPluginNext also includes is-next-router-error, which may duplicate this include.
Run:
🏁 Script executed:
#!/bin/bash
# See how other frameworks structure this and whether '>' is used project-wide
rg -n -C2 'optimizeViteDeps|optimizeDeps' code/ | sed -n '1,200p'
rg -nP '>\s*styled-jsx' code/Length of output: 7058
Replace unconventional '>’ entries with bare imports
Vite’s optimizeDeps.include won’t expand "@storybook/nextjs-vite-rsc > styled-jsx". In code/frameworks/nextjs-vite-rsc/src/preset.ts update:
'@storybook/nextjs-vite-rsc > styled-jsx'→'styled-jsx''@storybook/nextjs-vite-rsc > styled-jsx/style'→'styled-jsx/style'
Also remove the duplicate'next/dist/client/components/is-next-router-error'since it’s already included byvitestPluginNext.
🤖 Prompt for AI Agents
In code/frameworks/nextjs-vite-rsc/src/preset.ts around lines 38 to 44, the
optimizeViteDeps array contains unconventional entries using the " > " scoping
and a duplicate entry; replace '@storybook/nextjs-vite-rsc > styled-jsx' with
'styled-jsx' and '@storybook/nextjs-vite-rsc > styled-jsx/style' with
'styled-jsx/style', and remove the duplicate
'next/dist/client/components/is-next-router-error' entry so optimizeViteDeps
uses the bare imports and has no duplicates.
| import { createRouter } from '@storybook/nextjs-vite/router.mock'; | ||
| import { NextRouter } from '@storybook/nextjs-vite-rsc/rsc/client'; | ||
|
|
||
| import { isNextRouterError } from 'next/dist/client/components/is-next-router-error'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Deep import from Next internals is brittle; confirm stability or add fallback.
next/dist/client/components/is-next-router-error is not a public API and may change across Next 15.x.
🌐 Web query:
Is `next/dist/client/components/is-next-router-error` considered stable across Next.js 14/15, and what is the recommended way to detect Next router redirect/not-found errors?
💡 Result:
Short answer: No — import paths under next/dist (e.g. next/dist/client/components/is-next-router-error or other next/dist/* helpers) are internal implementation details and not a stable public API across major versions. Relying on them is fragile across Next.js 14 → 15 upgrades. [1][2][3]
Recommended approaches
- For redirects and not-found behaviour, use the public next/navigation functions: redirect(), permanentRedirect(), and notFound() and the documented app routing/error conventions. These functions intentionally throw special Next errors that the framework handles. Use them instead of importing internal helpers. [4][5]
- If you must catch and rethrow Next's internal redirect/not-found errors inside a try/catch, either:
- Re-throw when you detect the sentinel (examples in the docs/StackOverflow show checking the error message like "NEXT_REDIRECT"/"NEXT_NOT_FOUND"), or
- Use the community/work-in-progress helpers (isRedirectError / isNotFoundError) only knowing they come from next/dist and are not stable — prefer a pattern that rethrows Next errors rather than swallowing them. See the GitHub discussion requesting exported helpers. [6][7][1]
Concrete guidance
- Prefer the public APIs in next/navigation and the app router error conventions (error.js / not-found.js). Catching Next's special errors is an advanced workaround; if done, ensure you detect & rethrow Next's redirect/not-found errors so Next can perform the correct routing behaviour. [4][5][6]
References
- Next.js docs: redirect / notFound (public APIs). [4][5]
- Next.js docs: error handling / app router conventions. [3]
- Discussion / examples showing isRedirectError / isNotFoundError live under next/dist and community requests to export them. [6][7]
If you want, I can paste example code patterns for safely catching/rethrowing redirect/not-found errors.
Avoid deep import of Next internals: isNextRouterError from next/dist/* isn’t a public API and may break in Next 15. Use the public redirect(), permanentRedirect(), and notFound() functions from next/navigation (which throw Next’s special errors) or, if you must catch those errors, detect and rethrow based on the sentinel messages (NEXT_REDIRECT/NEXT_NOT_FOUND).
🤖 Prompt for AI Agents
In code/frameworks/nextjs-vite-rsc/src/preview.tsx around line 10, the file
imports isNextRouterError from next/dist/... which is a deep internal import and
must be removed; replace usage by relying on Next's public navigation helpers
(redirect, permanentRedirect, notFound) from next/navigation which throw the
framework sentinel errors, or if you need to catch those thrown errors
explicitly, detect them by matching the sentinel identifiers (e.g. check
error.message or a sentinel string for "NEXT_REDIRECT" / "NEXT_NOT_FOUND") and
rethrow non-sentinel errors—remove the internal import and update error handling
to use the public APIs or sentinel-string checks.
| export const decorators: Decorator[] = [ | ||
| (Story, context) => ( | ||
| <NextRouter> | ||
| <Story /> | ||
| </NextRouter> | ||
| ), |
There was a problem hiding this comment.
Decorator ignores story nextjs.navigation params; pass route/url to NextRouter.
Without wiring context.parameters.nextjs.navigation, stories can’t control the initial URL/path.
-export const decorators: Decorator[] = [
- (Story, context) => (
- <NextRouter>
- <Story />
- </NextRouter>
- ),
+export const decorators: Decorator[] = [
+ (Story, context) => {
+ const nav = context.parameters?.nextjs?.navigation ?? {};
+ const url = nav.url ?? (nav.pathname ? `${nav.pathname}${nav.search ?? ''}` : undefined);
+ return (
+ <NextRouter url={url} route={nav.pathname}>
+ <Story />
+ </NextRouter>
+ );
+ },Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In code/frameworks/nextjs-vite-rsc/src/preview.tsx around lines 56 to 61, the
Storybook decorator always renders <NextRouter> without passing the story's
nextjs.navigation parameters, so stories cannot control the initial route;
update the decorator to read context.parameters.nextjs?.navigation (or the
appropriate property name) and pass its route/url value into NextRouter (with a
sensible default when undefined) so each story can specify the initial path for
NextRouter.
| export const loaders: LoaderFunction<ReactRenderer> = async ({ globals, parameters }) => { | ||
| const { router, appDirectory } = parameters.nextjs ?? {}; | ||
| if (appDirectory) { | ||
| createNavigation(router); | ||
| } else { | ||
| createRouter({ | ||
| locale: globals.locale, | ||
| ...(router as Record<string, unknown>), | ||
| }); | ||
| } | ||
| initialize({ rootOptions: parameters.react?.rootOptions }); | ||
| }; |
There was a problem hiding this comment.
loaders must be an array; exporting a single function under loaders will not run.
Preview loaders is LoaderFunction[]. Exporting a bare function will likely be ignored at runtime.
-export const loaders: LoaderFunction<ReactRenderer> = async ({ globals, parameters }) => {
- initialize({ rootOptions: parameters.react?.rootOptions });
-};
+export const loaders: LoaderFunction<ReactRenderer>[] = [
+ async ({ parameters }) => {
+ initialize({ rootOptions: parameters.react?.rootOptions });
+ },
+];📝 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.
| export const loaders: LoaderFunction<ReactRenderer> = async ({ globals, parameters }) => { | |
| const { router, appDirectory } = parameters.nextjs ?? {}; | |
| if (appDirectory) { | |
| createNavigation(router); | |
| } else { | |
| createRouter({ | |
| locale: globals.locale, | |
| ...(router as Record<string, unknown>), | |
| }); | |
| } | |
| initialize({ rootOptions: parameters.react?.rootOptions }); | |
| }; | |
| export const loaders: LoaderFunction<ReactRenderer>[] = [ | |
| async ({ parameters }) => { | |
| initialize({ rootOptions: parameters.react?.rootOptions }); | |
| }, | |
| ]; |
🤖 Prompt for AI Agents
In code/frameworks/nextjs-vite-rsc/src/preview.tsx around lines 68 to 70, the
export currently exposes a single LoaderFunction but Preview expects loaders to
be an array (LoaderFunction[]); change the export to export an array containing
the async function (i.e., wrap the existing function in an array) so the runtime
will execute it; keep the function body as-is (initialize with rootOptions) and
ensure the exported name is still loaders.
| mountedRootEntries.push({ container, root }); | ||
| mountedContainers.add(container); | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Persist ownership flag.
- mountedRootEntries.push({ container, root });
+ mountedRootEntries.push({ container, root, createdByUs });📝 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.
| mountedRootEntries.push({ container, root }); | |
| mountedContainers.add(container); | |
| } else { | |
| mountedRootEntries.push({ container, root, createdByUs }); | |
| mountedContainers.add(container); | |
| } else { |
| return { | ||
| container, | ||
| baseElement, | ||
| unmount: root.unmount, | ||
| rerender: async (newUi) => { | ||
| ui = newUi; | ||
| await root.rerender(); | ||
| }, | ||
| asFragment: () => { | ||
| return document.createRange().createContextualFragment(container.innerHTML); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Unmount should also deregister the root and remove auto‑created containers to avoid leaks.
Currently unmount delegates to the client root only; the registry stays populated and containers remain in DOM if they weren’t attached to document.body.
- unmount: root.unmount,
+ unmount: async () => {
+ await root.unmount();
+ mountedContainers.delete(container);
+ const idx = mountedRootEntries.findIndex((it) => it.container === container);
+ if (idx !== -1) {
+ const [entry] = mountedRootEntries.splice(idx, 1);
+ if (entry.createdByUs && container.parentNode) {
+ container.parentNode.removeChild(container);
+ }
+ }
+ },📝 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.
| return { | |
| container, | |
| baseElement, | |
| unmount: root.unmount, | |
| rerender: async (newUi) => { | |
| ui = newUi; | |
| await root.rerender(); | |
| }, | |
| asFragment: () => { | |
| return document.createRange().createContextualFragment(container.innerHTML); | |
| }, | |
| }; | |
| return { | |
| container, | |
| baseElement, | |
| unmount: async () => { | |
| await root.unmount(); | |
| mountedContainers.delete(container); | |
| const idx = mountedRootEntries.findIndex((it) => it.container === container); | |
| if (idx !== -1) { | |
| const [entry] = mountedRootEntries.splice(idx, 1); | |
| if (entry.createdByUs && container.parentNode) { | |
| container.parentNode.removeChild(container); | |
| } | |
| } | |
| }, | |
| rerender: async (newUi) => { | |
| ui = newUi; | |
| await root.rerender(); | |
| }, | |
| asFragment: () => { | |
| return document.createRange().createContextualFragment(container.innerHTML); | |
| }, | |
| }; |
🤖 Prompt for AI Agents
In code/frameworks/nextjs-vite-rsc/src/testing-library.tsx around lines 76–87,
update the returned unmount so it not only calls root.unmount but also
deregisters the root from the internal registry and removes any auto-created
containers to prevent DOM leaks: after calling root.unmount(), remove the entry
for this root/container from the registry (e.g. delete from whatever Map or
registry holding roots), and if the container was created by the helper
(trackable via a flag/property on the element or by checking it is not attached
to document.body) remove it from the DOM; ensure the registry cleanup happens
even if root.unmount throws by performing deregistration/removal in a finally
block or equivalent.
| export async function cleanup() { | ||
| for (const { root, container } of mountedRootEntries) { | ||
| await root.unmount(); | ||
| if (container.parentNode === document.body) { | ||
| document.body.removeChild(container); | ||
| } | ||
| } | ||
| mountedRootEntries.length = 0; | ||
| mountedContainers.clear(); | ||
| } |
There was a problem hiding this comment.
cleanup doesn’t remove containers when baseElement ≠ document.body. Track ownership and always remove owned containers.
This can leave stray nodes when users pass a custom baseElement.
-export async function cleanup() {
- for (const { root, container } of mountedRootEntries) {
- await root.unmount();
- if (container.parentNode === document.body) {
- document.body.removeChild(container);
- }
- }
- mountedRootEntries.length = 0;
- mountedContainers.clear();
-}
+export async function cleanup() {
+ for (const { root, container, createdByUs } of mountedRootEntries) {
+ await root.unmount();
+ if (createdByUs && container.parentNode) {
+ container.parentNode.removeChild(container);
+ }
+ }
+ mountedRootEntries.length = 0;
+ mountedContainers.clear();
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In code/frameworks/nextjs-vite-rsc/src/testing-library.tsx around lines 90 to
99, cleanup only removes containers when their parent is document.body, which
leaves stray nodes if tests used a custom baseElement; update cleanup to remove
containers that this utility created regardless of which baseElement they were
appended to by checking ownership (e.g., consult mountedContainers or an owned
flag stored on each mountedRootEntry) and call parent.removeChild(container)
when the container is owned by this library, then clear mountedRootEntries and
mountedContainers as before.
| const client = await loadClient(); | ||
|
|
There was a problem hiding this comment.
Top‑level await on loadClient can break scan/build phases and triggers network access at import time.
During RSC scan builds (manager.isScanBuild) the virtual module returns a no‑op function; await loadClient() could yield undefined, breaking downstream calls. Also, importing this file will immediately make a dev‑server request. Lazily load and cache the client instead.
-const client = await loadClient();
+let clientPromise: Promise<typeof import('./react-client')> | null = null;
+async function getClient() {
+ return (clientPromise ??= loadClient());
+}Then replace calls:
- root = await client.createTestingLibraryClientRoot({
+ const client = await getClient();
+ root = await client.createTestingLibraryClientRoot({
container,
config,
fetchRsc,
});And:
- client.initialize();
+ (await getClient()).initialize();🤖 Prompt for AI Agents
In code/frameworks/nextjs-vite-rsc/src/testing-library.tsx around lines 113-114,
remove the top-level await of loadClient() because it triggers network access at
import time and can return undefined during RSC scan builds; instead implement a
lazy-loading, cached getter function (e.g., getClientAsync) that checks a cached
promise/value and only calls loadClient() on first demand, handles
manager.isScanBuild by returning the no-op fallback when loadClient() yields
undefined, and update all call sites to await this getter rather than using a
top-level client variable.
| ReactServer.setRequireModule({ | ||
| load: (id) => { | ||
| if (import.meta.env.__vite_rsc_build__) { | ||
| const import_ = buildServerReferences[id]; | ||
| if (!import_) { | ||
| throw new Error(`invalid server reference: ${id}`); | ||
| } | ||
| return import_(); | ||
| } else { | ||
| return __vite_rsc_raw_import__(/* @vite-ignore */ id); | ||
| } | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Confirm __vite_rsc_raw_import__ availability in dev.
If the Vite RSC plugin doesn’t define this helper in your dev environment, calls will fail. Consider falling back to dynamic import(/* @vite-ignore */ id) if unavailable.
- } else {
- return __vite_rsc_raw_import__(/* @vite-ignore */ id);
- }
+ } else {
+ const raw = (globalThis as any).__vite_rsc_raw_import__;
+ return raw ? raw(/* @vite-ignore */ id) : import(/* @vite-ignore */ id);
+ }To confirm:
🌐 Web query:
Does @vitejs/plugin-rsc expose a global function named __vite_rsc_raw_import__ at runtime for server-side module loading in dev?
💡 Result:
Short answer: No. @vitejs/plugin-rsc does not expose a runtime global named vite_rsc_raw_import; it provides the environment API (e.g. import.meta.viteRsc.loadModule) and the RSC runtime modules instead. See the plugin docs/README. [1][2]
Sources:
- @vitejs/plugin-rsc (npm) — environment API / import.meta.viteRsc.loadModule. [1]
- Vite plugins listing / plugin README (plugin-rsc description). [2]
Replace __vite_rsc_raw_import__ with import.meta.viteRsc.loadModule
@vitejs/plugin-rsc doesn’t expose a global __vite_rsc_raw_import__ at runtime. In dev, call import.meta.viteRsc.loadModule(id) and fall back to import(/* @vite-ignore */ id) if undefined.
- } else {
- return __vite_rsc_raw_import__(/* @vite-ignore */ id);
- }
+ } else {
+ const { viteRsc } = import.meta;
+ return viteRsc?.loadModule
+ ? viteRsc.loadModule(id)
+ : import(/* @vite-ignore */ id);
+ }📝 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.
| ReactServer.setRequireModule({ | |
| load: (id) => { | |
| if (import.meta.env.__vite_rsc_build__) { | |
| const import_ = buildServerReferences[id]; | |
| if (!import_) { | |
| throw new Error(`invalid server reference: ${id}`); | |
| } | |
| return import_(); | |
| } else { | |
| return __vite_rsc_raw_import__(/* @vite-ignore */ id); | |
| } | |
| }, | |
| }); | |
| ReactServer.setRequireModule({ | |
| load: (id) => { | |
| if (import.meta.env.__vite_rsc_build__) { | |
| const import_ = buildServerReferences[id]; | |
| if (!import_) { | |
| throw new Error(`invalid server reference: ${id}`); | |
| } | |
| return import_(); | |
| } else { | |
| const { viteRsc } = import.meta; | |
| return viteRsc?.loadModule | |
| ? viteRsc.loadModule(id) | |
| : import(/* @vite-ignore */ id); | |
| } | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In code/frameworks/nextjs-vite-rsc/src/testing-library.tsx around lines 118 to
130, the loader currently calls a non-existent global __vite_rsc_raw_import__;
update the else branch to first attempt import.meta.viteRsc.loadModule(id)
(guarding for undefined with optional chaining or a conditional) and, if that
returns undefined or the API is not present, fall back to dynamic import(/*
@vite-ignore */ id). Ensure the code uses the plugin-provided
import.meta.viteRsc.loadModule in dev and only uses the dynamic import as the
fallback.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
code/frameworks/nextjs-vite-rsc/src/rsc-plugin.ts (1)
218-220: Consider preserving some process.env valuesSetting
process.envto an empty object might break code that expects environment variables likeNODE_ENV. Consider preserving essential variables.define: { - 'process.env': JSON.stringify({}), + 'process.env.NODE_ENV': JSON.stringify(process.env.NODE_ENV || 'development'), + 'process.env': JSON.stringify({ NODE_ENV: process.env.NODE_ENV || 'development' }), __dirname: JSON.stringify(null), },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
code/frameworks/nextjs-vite-rsc/build-config.ts(2 hunks)code/frameworks/nextjs-vite-rsc/package.json(4 hunks)code/frameworks/nextjs-vite-rsc/src/index.ts(2 hunks)code/frameworks/nextjs-vite-rsc/src/rsc-plugin.ts(1 hunks)code/frameworks/nextjs-vite-rsc/src/rsc/headers.ts(1 hunks)code/frameworks/nextjs-vite-rsc/src/rsc/navigation.react-server.ts(1 hunks)code/frameworks/nextjs-vite-rsc/src/rsc/navigation.ts(1 hunks)code/frameworks/nextjs-vite-rsc/src/testing-library.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/frameworks/nextjs-vite-rsc/src/index.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/frameworks/nextjs-vite-rsc/src/rsc/navigation.tscode/frameworks/nextjs-vite-rsc/src/rsc-plugin.tscode/frameworks/nextjs-vite-rsc/src/rsc/navigation.react-server.tscode/frameworks/nextjs-vite-rsc/src/testing-library.tsxcode/frameworks/nextjs-vite-rsc/src/rsc/headers.tscode/frameworks/nextjs-vite-rsc/build-config.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/frameworks/nextjs-vite-rsc/src/rsc/navigation.tscode/frameworks/nextjs-vite-rsc/src/rsc-plugin.tscode/frameworks/nextjs-vite-rsc/src/rsc/navigation.react-server.tscode/frameworks/nextjs-vite-rsc/src/testing-library.tsxcode/frameworks/nextjs-vite-rsc/src/rsc/headers.tscode/frameworks/nextjs-vite-rsc/build-config.ts
🧠 Learnings (10)
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Group related mocks together
Applied to files:
code/frameworks/nextjs-vite-rsc/package.jsoncode/frameworks/nextjs-vite-rsc/build-config.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies
Applied to files:
code/frameworks/nextjs-vite-rsc/package.jsoncode/frameworks/nextjs-vite-rsc/build-config.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required dependencies that the test subject uses
Applied to files:
code/frameworks/nextjs-vite-rsc/package.jsoncode/frameworks/nextjs-vite-rsc/build-config.ts
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors
Applied to files:
code/frameworks/nextjs-vite-rsc/package.jsoncode/frameworks/nextjs-vite-rsc/build-config.ts
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Keep mock implementations simple and focused
Applied to files:
code/frameworks/nextjs-vite-rsc/build-config.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses
Applied to files:
code/frameworks/nextjs-vite-rsc/build-config.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases
Applied to files:
code/frameworks/nextjs-vite-rsc/build-config.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock at the highest level of abstraction needed
Applied to files:
code/frameworks/nextjs-vite-rsc/build-config.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in beforeEach blocks
Applied to files:
code/frameworks/nextjs-vite-rsc/build-config.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vi.mocked() to access and implement mock behaviors
Applied to files:
code/frameworks/nextjs-vite-rsc/build-config.ts
🧬 Code graph analysis (1)
code/frameworks/nextjs-vite-rsc/src/testing-library.tsx (2)
code/frameworks/nextjs-vite-rsc/src/react-client.tsx (4)
TestingLibraryClientRoot(14-14)FetchRsc(16-19)RscPayload(9-12)initialize(95-109)code/frameworks/nextjs-vite-rsc/src/load-client-dev.tsx (1)
loadClient(3-25)
⏰ 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: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (10)
code/frameworks/nextjs-vite-rsc/package.json (2)
14-14: Update homepage to the new package path.The homepage still links to the old
nextjs-vitepackage. Please point it at thenextjs-vite-rscdirectory to keep the published metadata accurate.- "homepage": "https://github.com/storybookjs/storybook/tree/next/code/frameworks/nextjs-vite", + "homepage": "https://github.com/storybookjs/storybook/tree/next/code/frameworks/nextjs-vite-rsc",
21-21: Fix the repository directory entry.
repository.directorystill referencescode/frameworks/nextjs. Please update it to the newcode/frameworks/nextjs-vite-rsclocation so tooling can resolve the correct folder.- "directory": "code/frameworks/nextjs" + "directory": "code/frameworks/nextjs-vite-rsc"code/frameworks/nextjs-vite-rsc/src/rsc-plugin.ts (2)
136-147: Ensure manager.isScanBuild and write flags are restored on errorIf a build throws, flags remain mutated, affecting subsequent builds. Wrap in try/finally.
async buildApp(builder) { const reactServer = builder.environments.client!; const reactClient = builder.environments['react_client']!; - manager.isScanBuild = true; - reactServer.config.build.write = false; - await builder.build(reactServer); - await builder.build(reactServer); - manager.isScanBuild = false; - reactServer.config.build.write = true; - await builder.build(reactClient); - await builder.build(reactServer); + const prevScan = manager.isScanBuild; + const prevWrite = reactServer.config.build.write; + manager.isScanBuild = true; + reactServer.config.build.write = false; + try { + await builder.build(reactServer); + await builder.build(reactServer); + manager.isScanBuild = false; + reactServer.config.build.write = true; + await builder.build(reactClient); + await builder.build(reactServer); + } finally { + manager.isScanBuild = prevScan; + reactServer.config.build.write = prevWrite; + } },
159-166: Hard-coded absolute public path breaks non-root base URLs
import("/storybook-static/react_client/index.mjs")fails whenbaseis not/(e.g., GitHub Pages, custom CDN path). Useimport.meta.env.BASE_URL.- return `export default async () => import("/storybook-static/react_client/index.mjs")`; + return `export default async () => import(/* @vite-ignore */ \`\${import.meta.env.BASE_URL}storybook-static/react_client/index.mjs\`)`;code/frameworks/nextjs-vite-rsc/src/testing-library.tsx (6)
12-16: Track container ownership to support correct unmount/cleanup behaviorAugment the registry to track whether we created the container.
-const mountedRootEntries: { - container: Container; - root: TestingLibraryClientRoot; -}[] = []; +const mountedRootEntries: { + container: Container; + root: TestingLibraryClientRoot; + createdByUs: boolean; +}[] = [];
36-37: Record whether the container was created by this helperThis flag drives proper cleanup.
- container ??= baseElement.appendChild(document.createElement('div')); + const createdByUs = !container; + container ??= baseElement.appendChild(document.createElement('div'));
68-79: Unmount should also deregister the root and remove auto-created containers to avoid leaksCurrently
unmountdelegates to the client root only; the registry stays populated and containers remain in DOM if they weren't attached todocument.body.- unmount: root.unmount, + unmount: async () => { + await root.unmount(); + mountedContainers.delete(container); + const idx = mountedRootEntries.findIndex((it) => it.container === container); + if (idx !== -1) { + const [entry] = mountedRootEntries.splice(idx, 1); + if (entry.createdByUs && container.parentNode) { + container.parentNode.removeChild(container); + } + } + },
82-91: cleanup doesn't remove containers when baseElement ≠ document.body. Track ownership and always remove owned containersThis can leave stray nodes when users pass a custom
baseElement.-export async function cleanup() { - for (const { root, container } of mountedRootEntries) { - await root.unmount(); - if (container.parentNode === document.body) { - document.body.removeChild(container); - } - } - mountedRootEntries.length = 0; - mountedContainers.clear(); -} +export async function cleanup() { + for (const { root, container, createdByUs } of mountedRootEntries) { + await root.unmount(); + if (createdByUs && container.parentNode) { + container.parentNode.removeChild(container); + } + } + mountedRootEntries.length = 0; + mountedContainers.clear(); +}
105-106: Top-level await on loadClient can break scan/build phases and triggers network access at import timeDuring RSC scan builds (
manager.isScanBuild) the virtual module returns a no-op function;await loadClient()could yieldundefined, breaking downstream calls. Also, importing this file will immediately make a dev-server request. Lazily load and cache the client instead.-const client = await loadClient(); +let clientPromise: Promise<typeof import('./react-client')> | null = null; +async function getClient() { + return (clientPromise ??= loadClient()); +}Then replace calls:
- root = await client.createTestingLibraryClientRoot({ + const client = await getClient(); + root = await client.createTestingLibraryClientRoot({ container, config, fetchRsc, });And:
- client.initialize(); + (await getClient()).initialize();
110-122: Replace__vite_rsc_raw_import__withimport.meta.viteRsc.loadModule
@vitejs/plugin-rscdoesn't expose a global__vite_rsc_raw_import__at runtime. In dev, callimport.meta.viteRsc.loadModule(id)and fall back toimport(/* @vite-ignore */ id)if undefined.} else { - return __vite_rsc_raw_import__(/* @vite-ignore */ id); + const { viteRsc } = import.meta; + return viteRsc?.loadModule + ? viteRsc.loadModule(id) + : import(/* @vite-ignore */ id); }
| server.middlewares.use(async (req, res, next) => { | ||
| const url = new URL(req.url ?? '/', 'https://any.local'); | ||
| if (url.pathname === '/@vite/invoke-react-client') { | ||
| const payload = JSON.parse(url.searchParams.get('data')!); | ||
| const result = await server.environments['react_client']!.hot.handleInvoke(payload); | ||
| res.setHeader('Content-Type', 'application/json'); | ||
| res.end(JSON.stringify(result)); | ||
| return; | ||
| } | ||
| next(); | ||
| }); |
There was a problem hiding this comment.
Add error handling for JSON parsing in middleware
The middleware directly parses JSON from query parameters without validation. This could crash the server if data is missing or malformed.
configureServer(server) {
server.middlewares.use(async (req, res, next) => {
const url = new URL(req.url ?? '/', 'https://any.local');
if (url.pathname === '/@vite/invoke-react-client') {
- const payload = JSON.parse(url.searchParams.get('data')!);
- const result = await server.environments['react_client']!.hot.handleInvoke(payload);
- res.setHeader('Content-Type', 'application/json');
- res.end(JSON.stringify(result));
- return;
+ try {
+ const data = url.searchParams.get('data');
+ if (!data) {
+ res.statusCode = 400;
+ res.end(JSON.stringify({ error: 'Missing data parameter' }));
+ return;
+ }
+ const payload = JSON.parse(data);
+ const result = await server.environments['react_client']!.hot.handleInvoke(payload);
+ res.setHeader('Content-Type', 'application/json');
+ res.end(JSON.stringify(result));
+ } catch (error) {
+ res.statusCode = 500;
+ res.setHeader('Content-Type', 'application/json');
+ res.end(JSON.stringify({ error: error.message }));
+ }
+ return;
}
next();
});
},📝 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.
| server.middlewares.use(async (req, res, next) => { | |
| const url = new URL(req.url ?? '/', 'https://any.local'); | |
| if (url.pathname === '/@vite/invoke-react-client') { | |
| const payload = JSON.parse(url.searchParams.get('data')!); | |
| const result = await server.environments['react_client']!.hot.handleInvoke(payload); | |
| res.setHeader('Content-Type', 'application/json'); | |
| res.end(JSON.stringify(result)); | |
| return; | |
| } | |
| next(); | |
| }); | |
| configureServer(server) { | |
| server.middlewares.use(async (req, res, next) => { | |
| const url = new URL(req.url ?? '/', 'https://any.local'); | |
| if (url.pathname === '/@vite/invoke-react-client') { | |
| try { | |
| const data = url.searchParams.get('data'); | |
| if (!data) { | |
| res.statusCode = 400; | |
| res.end(JSON.stringify({ error: 'Missing data parameter' })); | |
| return; | |
| } | |
| const payload = JSON.parse(data); | |
| const result = await server.environments['react_client']!.hot.handleInvoke(payload); | |
| res.setHeader('Content-Type', 'application/json'); | |
| res.end(JSON.stringify(result)); | |
| } catch (error) { | |
| res.statusCode = 500; | |
| res.setHeader('Content-Type', 'application/json'); | |
| res.end(JSON.stringify({ error: (error as Error).message })); | |
| } | |
| return; | |
| } | |
| next(); | |
| }); | |
| }, |
🤖 Prompt for AI Agents
In code/frameworks/nextjs-vite-rsc/src/rsc-plugin.ts around lines 114 to 124,
the middleware assumes the query param "data" exists and is valid JSON which can
crash the server; wrap retrieval and JSON.parse in a try/catch, first check that
url.searchParams.get('data') is non-null/non-empty, attempt to parse and if
parsing fails send a 400 response with Content-Type: application/json and a safe
error payload (e.g. { error: 'invalid_json' }) then return, and only call
server.environments['react_client']!.hot.handleInvoke when parsing succeeds;
ensure any unexpected exceptions are caught and result in a 500 response (or
next() as appropriate) to avoid crashing the process.
| const root = await client.createTestingLibraryClientRoot({ | ||
| container, | ||
| config, | ||
| fetchRsc, | ||
| }); |
There was a problem hiding this comment.
Add the root to mountedRootEntries for proper cleanup
The created root is never added to mountedRootEntries, so it won't be cleaned up by the cleanup() function.
const root = await client.createTestingLibraryClientRoot({
container,
config,
fetchRsc,
});
+ mountedRootEntries.push({ container, root, createdByUs });
+ mountedContainers.add(container);📝 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.
| const root = await client.createTestingLibraryClientRoot({ | |
| container, | |
| config, | |
| fetchRsc, | |
| }); | |
| const root = await client.createTestingLibraryClientRoot({ | |
| container, | |
| config, | |
| fetchRsc, | |
| }); | |
| mountedRootEntries.push({ container, root, createdByUs }); | |
| mountedContainers.add(container); |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/frameworks/nextjs-vite-rsc/src/rsc/headers.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/frameworks/nextjs-vite-rsc/src/rsc/headers.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/frameworks/nextjs-vite-rsc/src/rsc/headers.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: normal
- GitHub Check: Core Unit Tests, windows-latest
| const headersAdapter = new HeadersAdapter({}); | ||
| export const headers = () => headersAdapter; | ||
| export const cookies = () => RequestCookiesAdapter.seal(new RequestCookies(headersAdapter)); |
There was a problem hiding this comment.
Match Next 15’s promise-like headers/cookies contract.
In Next 15 the headers() and cookies() helpers are async Promise-like utilities—you can await them or pass them to React.use, while still calling methods synchronously thanks to the Promise being prototyped with the adapter. Returning the raw adapters here breaks the React.use(headers())/React.use(cookies()) pathway that the codemods now emit and that the docs recommend. (nextjs.org)
Please wrap the adapters in a thenable so we preserve both behaviors, e.g.:
-const headersAdapter = new HeadersAdapter({});
-export const headers = () => headersAdapter;
-export const cookies = () => RequestCookiesAdapter.seal(new RequestCookies(headersAdapter));
+const headersAdapter = new HeadersAdapter({});
+
+const wrapThenable = <T extends object>(value: T) =>
+ Object.assign(
+ value,
+ { then: (onFulfilled: (val: T) => unknown) => Promise.resolve(value).then(onFulfilled) }
+ );
+
+export const headers = () => wrapThenable(headersAdapter);
+
+export const cookies = () =>
+ wrapThenable(RequestCookiesAdapter.seal(new RequestCookies(headersAdapter)));This mirrors how Next implements the async upgrade: await headers() keeps working, React.use(headers()) succeeds, and existing synchronous call sites still see the adapter methods. (nextjs.org)
📝 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.
| const headersAdapter = new HeadersAdapter({}); | |
| export const headers = () => headersAdapter; | |
| export const cookies = () => RequestCookiesAdapter.seal(new RequestCookies(headersAdapter)); | |
| // code/frameworks/nextjs-vite-rsc/src/rsc/headers.ts | |
| const headersAdapter = new HeadersAdapter({}); | |
| const wrapThenable = <T extends object>(value: T) => | |
| Object.assign( | |
| value, | |
| { | |
| then: (onFulfilled: (val: T) => unknown) => | |
| Promise.resolve(value).then(onFulfilled), | |
| } | |
| ); | |
| export const headers = () => wrapThenable(headersAdapter); | |
| export const cookies = () => | |
| wrapThenable(RequestCookiesAdapter.seal(new RequestCookies(headersAdapter))); |
🤖 Prompt for AI Agents
In code/frameworks/nextjs-vite-rsc/src/rsc/headers.ts around lines 5 to 7, the
functions currently return raw adapter instances which breaks Next 15’s
promise-like contract; wrap each adapter in a thenable (a Promise that resolves
to the adapter and also exposes the adapter methods on the Promise object) so
callers can either await them or pass them into React.use while still calling
adapter methods synchronously. Concretely, create a Promise for each helper that
resolves to the adapter and assign the adapter’s methods/properties onto that
Promise (or prototype) so it is thenable and also immediately callable; ensure
cookies() uses the same headersAdapter instance when constructing the
RequestCookies so behavior stays consistent.
There was a problem hiding this comment.
why not
const wrapThenable = <T extends object>(value: T) => Object.assign(value, Promise.resolve(value));
There was a problem hiding this comment.
@kasperpeulen Good question! Your approach is more concise, but there's a subtle difference:
Your approach:
Object.assign(value, Promise.resolve(value))Copies all enumerable own properties from the Promise instance onto value. However, Promise methods like then, catch, finally are on Promise.prototype, not as own properties. So Object.assign won't actually copy them—you'd end up with just value plus whatever own properties the Promise instance has (which is typically none that are enumerable).
The original approach:
Object.assign(value, { then: (onFulfilled) => Promise.resolve(value).then(onFulfilled) })Explicitly adds a then method, which is the minimum requirement for a thenable. This is sufficient because:
React.use()only needs.then()to recognize it as a thenableawaitalso only requires.then()- The adapter methods remain directly callable on the object
Better alternative if you want all Promise methods:
const wrapThenable = <T extends object>(value: T) =>
Object.setPrototypeOf(value, Promise.resolve(value));This would make value inherit from a resolved Promise, giving you access to all Promise methods. However, this changes the prototype chain which might have unexpected side effects with the adapter's own prototype methods.
Recommendation: Stick with the explicit then approach—it's clear, minimal, and does exactly what's needed without potential prototype conflicts or confusion about what properties are being copied.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
code/frameworks/nextjs-vite-rsc/src/preview.tsx (4)
9-9: Deep import from Next.js internals is fragile across versions.
isNextRouterErroris imported fromnext/dist/client/components/is-next-router-error, which is an internal API that may break in Next.js updates. The public alternative is to useredirect(),permanentRedirect(), andnotFound()fromnext/navigation, which throw special errors, or detect these errors by checking for sentinel strings like"NEXT_REDIRECT"or"NEXT_NOT_FOUND"in the error message.Consider replacing with:
-import { isNextRouterError } from 'next/dist/client/components/is-next-router-error'; + +// Detect Next.js router errors by sentinel strings +function isNextRouterError(error: unknown): boolean { + if (typeof error === 'object' && error !== null) { + const message = String((error as any).message || ''); + return message.includes('NEXT_REDIRECT') || message.includes('NEXT_NOT_FOUND'); + } + return false; +}Based on learnings
67-69: Loader must be wrapped in an array to execute.The
loadersexport is typed as a singleLoaderFunction<ReactRenderer>, but Storybook's preview API expectsLoaderFunction<ReactRenderer>[]. The current code will not execute the loader at runtime.Apply this diff:
-export const loaders: LoaderFunction<ReactRenderer> = async ({ globals, parameters }) => { +export const loaders: LoaderFunction<ReactRenderer>[] = [async ({ globals, parameters }) => { initialize({ rootOptions: parameters.react?.rootOptions }); -}; +}];
100-105: Story function is incorrectly invoked as a React component.
React.createElement(storyFn, context)treats the story function as a component constructor and passescontextas props, rather than calling it as a function with the context argument. This breaks the story's access to args, parameters, and other context properties.Apply this diff:
applyDecorators: ( storyFn: LegacyStoryFn<ReactRenderer>, decorators: DecoratorFunction<ReactRenderer>[] ): LegacyStoryFn<ReactRenderer> => { - return defaultDecorateStory((context) => React.createElement(storyFn, context), decorators); + return defaultDecorateStory((context) => storyFn(context), decorators); },
107-122: Story function is incorrectly mounted as a React component.
unboundStoryFnis a story function that should be invoked withstoryContextto produce a React element. The current code treats it as a component (<Story {...storyContext} />), which spreads the context as props instead of passing it as a function argument, breaking the story's rendering.Apply this diff:
renderToCanvas: async function ( { storyContext, unboundStoryFn, showMain }: RenderContext<ReactRenderer>, canvasElement: ReactRenderer['canvasElement'] ) { - const Story = unboundStoryFn; - - const { unmount } = await renderServer(<Story {...storyContext} />, { + const element = unboundStoryFn(storyContext); + const { unmount } = await renderServer(element, { container: canvasElement, }); showMain(); return async () => { await unmount(); }; },code/frameworks/nextjs-vite-rsc/src/rsc/headers.ts (1)
1-17: Match Next 15’s thenable headers/cookies contract.Next 15 treats
headers()/cookies()as thenables; returning the raw adapters breaks codemoddedReact.use(headers())/await cookies(). Wrap the adapters in a thenable (and seal cookies) so both sync and async call sites behave like real Next.-import { RequestCookies } from 'next/dist/compiled/@edge-runtime/cookies'; +import { RequestCookies } from 'next/dist/compiled/@edge-runtime/cookies'; +import { RequestCookiesAdapter } from 'next/dist/server/web/spec-extension/adapters/request-cookies'; import { HeadersAdapter } from 'next/dist/server/web/spec-extension/adapters/headers'; import { fn } from 'storybook/test'; let headersAdapter = new HeadersAdapter({}); -let requestCookies = new RequestCookies(headersAdapter); +let requestCookies = RequestCookiesAdapter.seal(new RequestCookies(headersAdapter)); + +const wrapThenable = <T extends object>(value: T) => + Object.assign(value, { + then: (onFulfilled: (val: T) => unknown) => Promise.resolve(value).then(onFulfilled), + }); -export const cookies = fn(() => requestCookies); -export const headers = fn(() => headersAdapter); +export const cookies = fn(() => wrapThenable(requestCookies)); +export const headers = fn(() => wrapThenable(headersAdapter)); const originalRestore = cookies.mockRestore.bind(null); cookies.mockRestore = () => { originalRestore(); headersAdapter = new HeadersAdapter({}); - requestCookies = new RequestCookies(headersAdapter); + requestCookies = RequestCookiesAdapter.seal(new RequestCookies(headersAdapter)); };code/frameworks/nextjs-vite-rsc/src/rsc-plugin.ts (3)
121-128: Guard/@vite/invoke-react-clientJSON parsing.A bad/missing
dataquery currently crashes the dev server. Validate the param and handle parse errors so we return 4xx/5xx responses instead of throwing.server.middlewares.use(async (req, res, next) => { const url = new URL(req.url ?? '/', 'https://any.local'); if (url.pathname === '/@vite/invoke-react-client') { - const payload = JSON.parse(url.searchParams.get('data')!); - const result = await server.environments['react_client']!.hot.handleInvoke(payload); - res.setHeader('Content-Type', 'application/json'); - res.end(JSON.stringify(result)); - return; + try { + const data = url.searchParams.get('data'); + if (!data) { + res.statusCode = 400; + res.setHeader('Content-Type', 'application/json'); + res.end(JSON.stringify({ error: 'Missing data parameter' })); + return; + } + const payload = JSON.parse(data); + const result = await server.environments['react_client']!.hot.handleInvoke(payload); + res.setHeader('Content-Type', 'application/json'); + res.end(JSON.stringify(result)); + } catch (error) { + res.statusCode = 500; + res.setHeader('Content-Type', 'application/json'); + res.end( + JSON.stringify({ + error: error instanceof Error ? error.message : 'Unknown error', + }) + ); + } + return; } next(); });
146-154: Restore manager flags even when builds throw.If any build step rejects,
manager.isScanBuildandreactServer.config.build.writestay flipped, breaking the next build. Wrap the sequence in try/finally and reset the flags afterward.const reactServer = builder.environments.client!; const reactClient = builder.environments['react_client']!; - manager.isScanBuild = true; - reactServer.config.build.write = false; - await builder.build(reactServer); - await builder.build(reactServer); - manager.isScanBuild = false; - reactServer.config.build.write = true; - await builder.build(reactClient); - await builder.build(reactServer); + const prevScan = manager.isScanBuild; + const prevWrite = reactServer.config.build.write; + manager.isScanBuild = true; + reactServer.config.build.write = false; + try { + await builder.build(reactServer); + await builder.build(reactServer); + manager.isScanBuild = false; + reactServer.config.build.write = true; + await builder.build(reactClient); + await builder.build(reactServer); + } finally { + manager.isScanBuild = prevScan; + reactServer.config.build.write = prevWrite; + }
167-172: Make the load-client import base-aware.Hard-coding
/storybook-static/...breaks when Vite serves under a non-rootbase. Useimport.meta.env.BASE_URLso the generated import respects the configured base path.if (id === '\0virtual:vite-rsc-browser-mode/load-client') { if (manager.isScanBuild) { return `export default async () => {}`; } else { - return `export default async () => import("/storybook-static/react_client/index.mjs")`; + return `export default async () => import(/* @vite-ignore */ \`\${import.meta.env.BASE_URL}storybook-static/react_client/index.mjs\`)`; } }
🧹 Nitpick comments (1)
code/frameworks/nextjs-vite-rsc/src/index.ts (1)
13-13: Remove commented code.Commented-out exports create ambiguity. Since this is an intentional API change for the new
nextjs-vite-rscframework, remove the comment entirely.Apply this diff:
-// export * from '@storybook/react'; export * from './types';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
code/frameworks/nextjs-vite-rsc/src/index.ts(1 hunks)code/frameworks/nextjs-vite-rsc/src/preview.tsx(3 hunks)code/frameworks/nextjs-vite-rsc/src/rsc-plugin.ts(1 hunks)code/frameworks/nextjs-vite-rsc/src/rsc/headers.ts(1 hunks)code/frameworks/nextjs-vite-rsc/src/rsc/navigation.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/frameworks/nextjs-vite-rsc/src/rsc-plugin.tscode/frameworks/nextjs-vite-rsc/src/index.tscode/frameworks/nextjs-vite-rsc/src/rsc/navigation.tscode/frameworks/nextjs-vite-rsc/src/rsc/headers.tscode/frameworks/nextjs-vite-rsc/src/preview.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/frameworks/nextjs-vite-rsc/src/rsc-plugin.tscode/frameworks/nextjs-vite-rsc/src/index.tscode/frameworks/nextjs-vite-rsc/src/rsc/navigation.tscode/frameworks/nextjs-vite-rsc/src/rsc/headers.tscode/frameworks/nextjs-vite-rsc/src/preview.tsx
🧠 Learnings (1)
📚 Learning: 2025-09-24T09:39:39.233Z
Learnt from: ndelangen
PR: storybookjs/storybook#32507
File: code/core/src/manager/globals/globals-module-info.ts:25-33
Timestamp: 2025-09-24T09:39:39.233Z
Learning: In Storybook, storybook/actions/decorator is a preview-only entrypoint and should not be included in manager globals configuration. The duplicatedKeys array in code/core/src/manager/globals/globals-module-info.ts is specifically for manager-side externalization, not preview entrypoints.
Applied to files:
code/frameworks/nextjs-vite-rsc/src/preview.tsx
🧬 Code graph analysis (2)
code/frameworks/nextjs-vite-rsc/src/index.ts (4)
code/frameworks/nextjs-vite-rsc/src/vite-plugin/index.ts (1)
storybookNextJsPlugin(7-7)code/core/src/csf/csf-factories.ts (2)
PreviewAddon(62-63)InferTypes(32-34)code/frameworks/nextjs-vite-rsc/src/preview.tsx (1)
preview(125-125)code/frameworks/nextjs-vite-rsc/src/types.ts (1)
NextJsTypes(62-64)
code/frameworks/nextjs-vite-rsc/src/preview.tsx (2)
code/renderers/react/src/types.ts (1)
ReactRenderer(8-12)code/frameworks/nextjs-vite-rsc/src/testing-library.tsx (2)
initialize(107-125)renderServer(17-80)
⏰ 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: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (4)
code/frameworks/nextjs-vite-rsc/src/index.ts (3)
2-2: LGTM! Alias resolves naming conflict.The import alias
definePreviewBasecorrectly avoids shadowing the exporteddefinePreviewfunction while maintaining clear intent.
18-20: Module path correction looks good.The module declaration now correctly references
@storybook/nextjs-vite-rsc/vite-plugin, fixing the previous mismatch flagged in earlier review.
26-26: LGTM! Consistent with import alias.The call to
definePreviewBasecorrectly matches the aliased import and maintains the intended composition pattern.code/frameworks/nextjs-vite-rsc/src/preview.tsx (1)
90-98: LGTM: Standard Preview render implementation.The render function correctly extracts the component from context, validates its presence, and renders it with args.
| const originalRestore = cookies.mockRestore.bind(null); | ||
|
|
||
| cookies.mockRestore = () => { | ||
| originalRestore(); | ||
| headersAdapter = new HeadersAdapter({}); | ||
| requestCookies = new RequestCookies(headersAdapter); | ||
| }; |
There was a problem hiding this comment.
Preserve mockRestore’s this binding.
Binding the original mockRestore to null makes it throw when invoked (Vitest/Jest expect the mock as this). Call it with the mock instance instead so restores keep working.
-const originalRestore = cookies.mockRestore.bind(null);
+const originalRestore = cookies.mockRestore;
cookies.mockRestore = () => {
- originalRestore();
+ originalRestore.call(cookies);
headersAdapter = new HeadersAdapter({});
requestCookies = new RequestCookies(headersAdapter);
};📝 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.
| const originalRestore = cookies.mockRestore.bind(null); | |
| cookies.mockRestore = () => { | |
| originalRestore(); | |
| headersAdapter = new HeadersAdapter({}); | |
| requestCookies = new RequestCookies(headersAdapter); | |
| }; | |
| const originalRestore = cookies.mockRestore; | |
| cookies.mockRestore = () => { | |
| originalRestore.call(cookies); | |
| headersAdapter = new HeadersAdapter({}); | |
| requestCookies = new RequestCookies(headersAdapter); | |
| }; |
🤖 Prompt for AI Agents
In code/frameworks/nextjs-vite-rsc/src/rsc/headers.ts around lines 11 to 17, the
code binds cookies.mockRestore to null which loses the mock’s this context and
breaks restore; change the call to preserve the mock instance as this (either
bind to cookies instead of null or invoke originalRestore with .call(cookies))
so mockRestore executes with the correct this and then continue recreating
headersAdapter and requestCookies.
|
Failed to publish canary version of this pull request, triggered by @kasperpeulen. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/18189523440 |
|
Failed to publish canary version of this pull request, triggered by @kasperpeulen. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/18191164519 |
1 similar comment
|
Failed to publish canary version of this pull request, triggered by @kasperpeulen. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/18191164519 |
|
Failed to publish canary version of this pull request, triggered by @kasperpeulen. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/18192573126 |
1 similar comment
|
Failed to publish canary version of this pull request, triggered by @kasperpeulen. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/18192573126 |
|
Failed to publish canary version of this pull request, triggered by @kasperpeulen. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/18195595543 |
|
Failed to publish canary version of this pull request, triggered by @shilman. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/18210036451 |
|
Failed to publish canary version of this pull request, triggered by @shilman. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/18213683320 |
There was a problem hiding this comment.
Actionable comments posted: 30
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
code/core/src/manager/components/sidebar/HighlightStyles.tsx (1)
9-10: Remove the@ts-expect-errorand fix the component’s prop type.The imported
Highlightis defined asItemRef | null, so destructuringrefId/itemIdtriggers a type error. Narrow the props toItemRef(e.g.FC<ItemRef>) or guard againstnullbefore destructuring.code/core/src/common/utils/sync-main-preview-addons.test.ts (1)
12-12: Missingspy: truein vi.mock() call.Per coding guidelines for Vitest tests, use
vi.mock()with thespy: trueoption for all mocks.As per coding guidelines.
Apply this diff:
-vi.mock('./get-addon-annotations'); +vi.mock('./get-addon-annotations', { spy: true });code/core/src/components/components/tooltip/WithTooltip.tsx (1)
178-224: Specify dependencies for theuseEffecthook
The effect currently has no dependency array—causing it to re-run on every render and risking stale closures or performance issues. AddonVisibilityChangeto its dependencies:- }); + }, [onVisibilityChange]);code/addons/docs/src/blocks/components/Preview.tsx (1)
157-165: Add type guards and return type annotation.The function has type safety concerns:
- The cast to
ReactElementat line 159 is unsafe without validating thatchildrenis actually a valid React element- Missing explicit return type annotation
- The props check at line 160 doesn't prevent accessing invalid elements
Apply this diff to improve type safety:
-function getChildProps(children: ReactNode) { +function getChildProps(children: ReactNode): Record<string, any> | null { if (Children.count(children) === 1) { - const elt = children as ReactElement; - if (elt.props) { + if (React.isValidElement(children)) { + const elt = children; + if (elt.props) { - return elt.props; + return elt.props; + } } } return null; }This adds proper element validation using
React.isValidElement()and an explicit return type.MIGRATION.md (1)
570-579: Convert theimport.meta.resolve()result before assigning it tocore.builder.
import.meta.resolve()returns a file URL (e.g.file:///.../index.js), not a plain filesystem path. Passing that URL straight intocore.builderwill violate the “fully resolved path” requirement and break any filesystem operations Storybook performs on it. Please update the snippet to convert the URL to a path first (for example withfileURLToPath(new URL('@storybook/builder-vite', import.meta.url))) before assigning it tocore.builder.code/core/src/preview-api/modules/store/StoryStore.ts (1)
288-341: Restore correct extract output and drop debug logsTwo problems here:
- The new
console.logcalls will spam consumer builds; please remove them.- Setting
storyIdtoentry.parentrewrites every story’s exported ID to its component ID (since normal stories always have a parent component). That breaks consumers relying on the actual story ID. KeepstoryIdas the originalstoryId, only overriding it when you truly need special handling (e.g., forentry.subtype === 'test').
♻️ Duplicate comments (1)
code/core/src/common/js-package-manager/BUNProxy.ts (1)
8-8: Verify the 'empathic/find' package exists.This import has the same issue as noted in
JsPackageManager.tsline 6. Theempathic/findpackage could not be found in public npm registries. Please ensure the package is available before this code can run.
🧹 Nitpick comments (33)
code/addons/docs/src/blocks/blocks/external/ExternalPreview.ts (1)
78-78: Address the TODO: clarify story test handling requirements.The TODO comment indicates uncertain requirements for story test handling. This gap could result in:
- Story tests not being indexed or registered properly
- Missing change notifications when story tests are added/modified
- Incomplete integration with the broader story test feature
Please clarify:
- Should story tests be added to
storyIndex.entrieswith a differentsubtype(e.g.,'test')?- Should story tests trigger a separate notification mechanism?
- Is this functionality deferred to a future PR, or should it be implemented now?
Would you like me to:
- Generate a proposed implementation for story test handling based on patterns from related files?
- Open a tracking issue for this TODO if the work is deferred?
- Search the codebase for existing story test handling patterns to inform the implementation?
code/core/src/actions/loaders.ts (1)
20-23: Optional: Clarify comment to distinguish default vs. explicit empty-name cases.The comment "Default name provided by vi.fn(), which we don't want to log." is accurate but could be slightly more precise to contrast with the next skip condition (lines 25-29). The 'spy' name appears when
vi.fn()is called without arguments, whereas 'vi.fn()' appears whenmockName('')is explicitly set.Consider rephrasing to:
- // Default name provided by vi.fn(), which we don't want to log. + // Default name when vi.fn() is called without a name argument. if (name === 'spy') { return; }This makes the distinction between the two skip conditions more explicit for maintainers.
code/addons/vitest/src/vitest-plugin/viewports.ts (2)
77-81: Clarify the semantic distinction betweenviewportsandoptionsproperties.The
ViewportsParaminterface defines bothviewportsandoptionsasViewportMap, and they're merged here withoptionstaking precedence. However, there's no clear documentation explaining:
- The intended semantic difference between these two properties
- Why users would provide both
- The precedence order (currently
options>viewports>MINIMAL_VIEWPORTS)Additionally, naming the merged result
optionscreates confusion sinceoptionsis also a property name inviewportsParam.Consider one of the following:
Document the distinction in the
ViewportsParaminterface with JSDoc comments explaining when to use each property.Use a clearer name for the merged variable:
- const options: ViewportMap = { + const mergedViewports: ViewportMap = { ...MINIMAL_VIEWPORTS, ...viewportsParam.viewports, ...viewportsParam.options, };
- If the distinction isn't meaningful, consider deprecating one property and standardizing on a single approach.
86-87: Consider reverting the destructuring change for clarity.The change from direct property access to destructuring doesn't improve readability or add value:
// Current (after change) const { styles } = options[defaultViewport]; // Previous (before change, implied) const styles = viewports[defaultViewport].styles;The direct property access is more concise and equally clear. The destructuring pattern is typically beneficial when extracting multiple properties, but here only
stylesis used.If you prefer consistency with the rename to
options, consider:- const { styles } = options[defaultViewport]; + const styles = options[defaultViewport].styles;This maintains clarity while reflecting the variable rename.
code/addons/a11y/src/components/A11yContext.tsx (1)
398-398: Consider depending onstatusdirectly instead ofisInitial.The dependency array now includes
isInitial, which is derived fromstatus. While functionally correct, React best practices recommend including the source state (status) in the dependency array rather than a derived value. This makes dependencies explicit and aligns with the exhaustive-deps rule.You could refactor by either:
- Adding
statusto the dependency array and removingisInitial, then usingstatus === 'initial'directly in the guard condition, or- Computing
isInitialinside the effect body if you prefer the named constant for readability.Example (option 1):
- const isInitial = status === 'initial'; - useEffect(() => { emit(REMOVE_HIGHLIGHT, `${ADDON_ID}/selected`); emit(REMOVE_HIGHLIGHT, `${ADDON_ID}/others`); - if (!highlighted || isInitial) { + if (!highlighted || status === 'initial') { return; } // ... rest of effect - }, [isInitial, emit, highlighted, results, tab, selectedItems]); + }, [status, emit, highlighted, results, tab, selectedItems]);code/core/src/controls/components/SaveStory.stories.tsx (1)
45-50: Verify the switch fromuserEventtofireEventis intentional.The change from
userEvent.type()tofireEvent.change()simplifies the test but reduces realism:
userEvent.type()simulates character-by-character typing with full event sequences (focus, keydown, keypress, input, keyup)fireEvent.change()directly sets the input value without intermediate eventsImplications:
- If the
SaveStorycomponent has keystroke-level validation, onChange handlers that fire per character, or other intermediate event logic, this test will no longer catch related bugs- The test is now faster and simpler but less representative of actual user interaction
Recommendation:
If the component only validates on submit (not per keystroke),fireEvent.change()is acceptable. Otherwise, consider keepinguserEventfor more thorough interaction testing.Can you confirm whether the
SaveStorycomponent has any keystroke-level validation or onChange behavior that should be tested? If so, consider reverting touserEvent:- play: async ({ canvas, context }) => { + play: async ({ canvas, context, userEvent }) => { await Creating.play(context); const dialog = await canvas.findByRole('dialog'); const input = await within(dialog).findByRole('textbox'); - await fireEvent.change(input, { target: { value: 'MyNewStory' } }); + await userEvent.type(input, 'MyNewStory'); await fireEvent.submit(dialog.getElementsByTagName('form')[0]); await expect(context.args.createStory).toHaveBeenCalledWith('MyNewStory'); },code/core/src/preview-api/modules/store/args.test.ts (1)
127-136: ReactNode mapping matches implementation; add edge-case testsThe
othercase in args.ts (lines 58–66) usesisPrimitiveArg(string│number│boolean) for ReactNode and filters everything else. Current tests align with this logic. Consider adding edge-case assertions for null, undefined, arrays, NaN, and Infinity to fully document expected behavior..github/copilot-instructions.md (2)
17-34: Add a language tag to the repo-structure code fence.Markdownlint flags this fenced block because it lacks a language identifier. Add something like
textafter the opening backticks to appease tooling and keep formatting consistent. Based on static analysis hints.
161-236: Wrap bare URLs in angle brackets.Markdownlint warns about the plain
http://localhost:6006/references. Wrapping them as<http://localhost:6006/>keeps the linter quiet without changing the rendered output. Based on static analysis hints.code/core/src/components/components/tooltip/WithTooltip.tsx (1)
180-184: Consider preventing default Escape key behavior.The Escape key handler will close the tooltip but allows the event to propagate. For better control and to prevent potential conflicts with other Escape key handlers, consider calling
preventDefault().const handleKeyDown = (e: KeyboardEvent) => { if (e.key === 'Escape') { + e.preventDefault(); hide(); } };code/core/src/component-testing/components/test-fn.stories.tsx (1)
132-177: Consider documenting the empty test bodies.The TestNames story and its tests effectively demonstrate test name handling with descriptive error scenarios. However, the empty test bodies could benefit from either:
- A comment explaining they're intentional placeholders for framework validation
- Using test framework skip/todo markers (e.g.,
test.todo()ortest.skip())This would make the intent clearer to future maintainers and prevent confusion about incomplete tests.
Example with comments:
TestNames.test( 'should display an error when login is attempted with an expired session token', - () => {} + () => { + // Placeholder: validates test name handling in framework + } );code/addons/onboarding/src/Onboarding.tsx (3)
85-86: Optimize by movinguserAgentoutside the component.The
userAgentvalue is static and does not change during a user's session, but it's currently re-read on every render. Move the declaration outside the component to avoid redundant reads.Apply this diff to move the declaration:
+// eslint-disable-next-line compat/compat +const userAgent = globalThis?.navigator?.userAgent; + export default function Onboarding({ api }: { api: API }) { const [enabled, setEnabled] = useState(true); const [showConfetti, setShowConfetti] = useState(false); const [step, setStep] = useState<StepKey>('1:Intro'); const [primaryControl, setPrimaryControl] = useState<HTMLElement | null>(); const [saveFromControls, setSaveFromControls] = useState<HTMLElement | null>(); const [createNewStoryForm, setCreateNewStoryForm] = useState<HTMLElement | null>(); const [createdStory, setCreatedStory] = useState<{ newStoryName: string; newStoryExportName: string; sourceFileContent: string; sourceFileName: string; } | null>(); - // eslint-disable-next-line compat/compat - const userAgent = globalThis?.navigator?.userAgent; - const selectStory = useCallback(
127-127: Dependency array can be simplified ifuserAgentis moved outside.The
userAgentdependency is currently correct. However, if you moveuserAgentoutside the component as suggested earlier, it becomes a stable reference and can be removed from this dependency array.
186-186: Dependency array can be simplified ifuserAgentis moved outside.The
userAgentdependency is currently correct. However, if you moveuserAgentoutside the component as suggested earlier, it becomes a stable reference and can be removed from this dependency array.code/core/src/manager/components/sidebar/ContextMenu.tsx (3)
76-84: Potential memory leak with setTimeout.If the component unmounts before the 2-second timeout completes, the
setCopyTextcall will attempt to update unmounted component state. This won't cause errors in React but is considered a memory leak and bad practice.Apply this pattern to clean up the timeout:
const topLinks = useMemo<Link[]>(() => { + let timeoutId: NodeJS.Timeout | null = null; const defaultLinks = []; if (context && 'importPath' in context) { // ... open in editor link } if (context.type === 'story') { defaultLinks.push({ id: 'copy-story-name', title: copyText, icon: <CopyIcon />, onClick: (e: SyntheticEvent) => { e.preventDefault(); copy(context.exportName); setCopyText('Copied!'); - setTimeout(() => { + if (timeoutId) clearTimeout(timeoutId); + timeoutId = setTimeout(() => { setCopyText('Copy story name'); + timeoutId = null; }, 2000); }, }); } return defaultLinks; }, [context, copyText, enableShortcuts, shortcutKeys]);However, this still doesn't handle unmounting. A better approach is to use
useEffectwith cleanup or use a ref to track mounted state.
173-174: Add runtime safety check for links parameter.If
linksisundefinedor not an array, accessinglinks[0]orlinks.lengthwill throw a runtime error.Apply this diff to add a safety check:
+ const safeLinks = links || []; const groups: Link[][] = - Array.isArray(links[0]) || links.length === 0 ? (links as Link[][]) : [links as Link[]]; + Array.isArray(safeLinks[0]) || safeLinks.length === 0 + ? (safeLinks as Link[][]) + : [safeLinks as Link[]];Alternatively, update the function signature to ensure
linksis always defined:const LiveContextMenu: FC<{ context: API_HashEntry; links?: Link[] | Link[][] } & ComponentProps<typeof TooltipLinkList>>Then use:
const safeLinks = links || [];
173-174: Type assertion bypasses TypeScript safety.The type assertions
as Link[][]andas Link[]suppress TypeScript's type checking, which could hide type mismatches. Consider using a type guard function or refining the input type to avoid assertions.Consider this alternative without type assertions:
const groups: Link[][] = (() => { if (!links || links.length === 0) return []; if (Array.isArray(links[0])) return links as Link[][]; return [links] as Link[][]; })();Or define a type guard:
function isNestedLinkArray(links: Link[] | Link[][]): links is Link[][] { return links.length > 0 && Array.isArray(links[0]); } const groups: Link[][] = isNestedLinkArray(links) ? links : [links];code/core/src/csf-tools/vitest-plugin/transformer.test.ts (1)
1008-1089: Refresh the skipped remapping test to the new transform shapeThe
it.skip('should remap the location of story tests', …)block is still wired to the pre-change output (_test("render test"…, no auto-calculated title slug, no base-story describe suffix). With today’s transformer it would throw as soon as the skip is lifted because the lookup strings are no longer present and the snapshot is stale. Please update the snapshot and the string searches (render test) to mirror the current output (e.g.base story,automatic-calculated-title--primary…) so the test is ready to unskip.code/core/src/cli/detect.test.ts (1)
24-24: Consider addingspy: trueoption to the mock.The coding guidelines recommend using
vi.mock()with thespy: trueoption for all package and file mocks in Vitest tests to enable spy functionality and ensure consistent mocking behavior. While this mock may work without it, addingspy: truealigns with the established pattern.As per coding guidelines.
Apply this diff:
-vi.mock('empathic/find'); +vi.mock('empathic/find', () => ({ + up: vi.fn(), +}), { spy: true });Note: If you don't need to spy on or implement behaviors for the mocked functions in this test, the current approach is acceptable.
code/core/src/cli/helpers.test.ts (1)
56-58: Consider addingspy: trueoption to the mock.The coding guidelines recommend using
vi.mock()with thespy: trueoption for all package and file mocks in Vitest tests. While the current mock implementation provides a factory function, addingspy: trueensures consistent spy behavior across all mocks.As per coding guidelines.
Apply this diff:
-vi.mock('empathic/find', () => ({ - up: vi.fn(), -})); +vi.mock('empathic/find', () => ({ + up: vi.fn(), +}), { spy: true });code/.storybook/main.ts (1)
139-139: Document the purpose of the experimental flag.The
experimentalTestSyntax: trueflag is added without inline documentation explaining what test syntax features it enables or what behavior changes. Consider adding a comment describing the flag's purpose for future maintainers.Example:
features: { developmentModeForBuild: true, + // Enable experimental test syntax support (e.g., new test decorators, test tags) experimentalTestSyntax: true, },code/core/src/component-testing/components/Subnav.stories.tsx (1)
1-1: React import may be unnecessary.With the modern JSX transform (React 17+), explicit
Reactimports are no longer required in files that only use JSX. If your build configuration supports the new JSX transform, this import can be removed.Verify your build configuration:
#!/bin/bash # Check if tsconfig.json or build config uses the new JSX transform rg -n "jsx.*react-jsx" tsconfig.jsoncode/core/src/csf/index.ts (1)
30-32: Consider validating theparentIdparameter.The
toTestIdfunction sanitizestestNamebut notparentId. IfparentIdcontains invalid characters or is not a properly formatted story ID, the resulting test ID may be invalid.Consider one of these approaches:
- Document the assumption that
parentIdmust be a valid story ID:-/** Generate a storybook test ID from a story ID and test name. */ +/** + * Generate a storybook test ID from a story ID and test name. + * @param parentId - A valid story ID (must be pre-sanitized) + * @param testName - The test name to sanitize and append + */ export const toTestId = (parentId: string, testName: string) => `${parentId}:${sanitizeSafe(testName, 'test')}`;
- Or validate
parentIdformat:export const toTestId = (parentId: string, testName: string) => { + if (!parentId || typeof parentId !== 'string') { + throw new Error(`Invalid parentId '${parentId}', must be a valid story ID`); + } return `${parentId}:${sanitizeSafe(testName, 'test')}`; }code/core/src/manager/components/sidebar/NoResults.tsx (1)
3-17: Treat text-wrap: 'balance' as progressive enhancement
Supported in Chrome 130+, Edge 130+, Firefox 121+, Safari 17.5+; add fallback (e.g., remove or override the rule) for older or mobile browsers if broad compatibility is required.code/core/src/cli/detect.ts (1)
173-175: Consider addinglastoption for bounded search.The
detectPnpfunction searches for PnP files without an upper boundary, which could search all the way to the filesystem root. For consistency with other searches in this file and to avoid unintended matches outside the project, consider adding{ last: getProjectRoot() }.Apply this diff:
export async function detectPnp() { - return !!find.any(['.pnp.js', '.pnp.cjs']); + return !!find.any(['.pnp.js', '.pnp.cjs'], { last: getProjectRoot() }); }code/core/src/common/utils/__tests__/paths.test.ts (1)
60-60: Remove unnecessary type casts in mock conditions.The
as anycasts for.svnand.yarnare unnecessary and should be removed for consistency with the.gitcheck.Apply this diff:
it('should return the root directory containing a .svn directory if there is no .git directory', () => { vi.mocked(find.up).mockImplementation((name) => - name === ('.svn' as any) ? '/path/to/root' : undefined + name === '.svn' ? '/path/to/root' : undefined ); expect(slash(getProjectRoot())).toBe('/path/to'); }); it('should return the root directory containing a .yarn directory if there is no .git or .svn directory', () => { vi.mocked(find.up).mockImplementation((name) => - name === ('.yarn' as any) ? '/path/to/root' : undefined + name === '.yarn' ? '/path/to/root' : undefined ); expect(slash(getProjectRoot())).toBe('/path/to'); });Also applies to: 68-68
code/core/src/manager/components/preview/tools/open-in-editor.tsx (1)
32-54: Consider passing api through Consumer to avoid redundant hook call.The current implementation works, but calling
useStorybookApi()inside the Consumer render function (line 34) is redundant since theapiis already available in themapperfunction'sComboparameter (line 12). You could includeapiin the mapper's return value to avoid the extra hook call.Apply this diff to streamline the implementation:
const mapper = ({ state, api }: Combo) => { const { storyId, refId } = state; const entry = api.getData(storyId, refId); const isCompositionStory = !!refId; return { storyId, isCompositionStory, importPath: entry?.importPath as string | undefined, + api, }; }; export const openInEditorTool: Addon_BaseType = { title: 'open-in-editor', id: 'open-in-editor', type: types.TOOL, match: ({ viewMode, tabId }) => global.CONFIG_TYPE === 'DEVELOPMENT' && (viewMode === 'story' || viewMode === 'docs') && !tabId, render: () => ( <Consumer filter={mapper}> - {({ importPath, isCompositionStory }) => { - const api = useStorybookApi(); + {({ importPath, isCompositionStory, api }) => { if (isCompositionStory || !importPath) { return null; }code/core/src/cli/eslintPlugin.test.ts (1)
17-19: Addspy: trueto the new mockOur Vitest guidelines require
vi.mock()calls for packages/files to set{ spy: true }so we can safely access them viavi.mocked. Please update this mock accordingly.As per coding guidelines.
-vi.mock('empathic/find', () => ({ - up: vi.fn(), -})); +vi.mock( + 'empathic/find', + () => ({ + up: vi.fn(), + }), + { spy: true } +);code/core/src/core-server/utils/StoryIndexGenerator.ts (1)
444-460: Default subtype when indexers omit itBefore this change,
createIndeximplementations (internal and external) only emittedtype, so plenty of indexers still return story entries without asubtype. Serializing them here will now producesubtype: undefined, while the rest of the pipeline (and the types introduced in this PR) expect'story' | 'test'. Please keep the old behavior by defaulting to'story'when the field is missing so existing indexers don’t start emittingundefined.- type: 'story', - subtype: input.type === 'story' ? input.subtype : 'story', + type: 'story', + subtype: input.type === 'story' ? input.subtype ?? 'story' : 'story',code/core/src/csf-tools/vitest-plugin/transformer.ts (1)
34-44: Consider renaming the helper function.The function
getLiteralWithZeroWidthSpaceactually appends double spaces (DOUBLE_SPACES), not zero-width spaces. This is misleading. Consider renaming to something likegetLiteralWithSpaceSuffixorgetLiteralWithDoubleSpacesto accurately reflect the implementation.Apply this diff to rename the helper:
-const getLiteralWithZeroWidthSpace = (testTitle: string) => +const getLiteralWithDoubleSpaces = (testTitle: string) => t.stringLiteral(`${testTitle}${DOUBLE_SPACES}`);And update the usage on line 263:
const describeBlock = t.callExpression(vitestDescribeId, [ - getLiteralWithZeroWidthSpace(describeTitle), + getLiteralWithDoubleSpaces(describeTitle), t.arrowFunctionExpression(code/core/src/preview-api/modules/store/StoryStore.test.ts (2)
11-31: Align Vitest mocks with project guidelines (spy: true, vi.mocked, resets).
- Add spy: true to vi.mock to auto‑spy actual exports.
- Use vi.mocked() when asserting calls (typing + clarity).
- Reset/clear mocks in beforeEach to avoid cross‑test leakage.
Apply these diffs:
-vi.mock('./csf/prepareStory', async (importOriginal) => { +vi.mock('./csf/prepareStory', async (importOriginal) => { const actual = await importOriginal<typeof import('./csf/prepareStory')>(); return { ...actual, prepareStory: vi.fn(actual.prepareStory), }; -}); +}, { spy: true }); -vi.mock('./csf/processCSFFile', async (importOriginal) => ({ +vi.mock('./csf/processCSFFile', async (importOriginal) => ({ processCSFFile: vi.fn( (await importOriginal<typeof import('./csf/processCSFFile')>()).processCSFFile ), -})); +}), { spy: true }); -vi.mock('@storybook/global', async (importOriginal) => ({ +vi.mock('@storybook/global', async (importOriginal) => ({ global: { ...(await importOriginal<typeof import('@storybook/global')>()), }, -})); +}), { spy: true });Also add a global reset:
// Near the top-level describe('StoryStore', ...) beforeEach(() => { vi.clearAllMocks(); });And prefer
vi.mocked(processCSFFile)/vi.mocked(prepareStory)in assertions for type-safety.
As per coding guidelines
289-357: Snapshots updated — consider reducing brittleness.Inline snapshots look consistent. To reduce churn, consider asserting targeted fields (e.g., id, name, parameters.fileName) instead of full objects.
Also applies to: 482-682
code/core/src/preview-api/modules/preview-web/render/StoryRender.ts (1)
118-128: Abort phase gating prevents duplicate emissions — good change.Only emitting/setting 'aborted' when not in a terminal phase avoids redundant phase changes.
For consistency, prefer calling
this.checkIfAborted(abortSignal)instead of rawabortSignal.abortedchecks (e.g., Lines 291, 308–311, 376–378) so phase/event handling is centralized.
| yarn i | ||
| # Time: ~2.5 minutes | ||
| # Timeout: Use 300+ seconds for bash commands | ||
| ``` |
There was a problem hiding this comment.
Fix install command: yarn i fails under Yarn 4.
yarn i isn’t a recognized alias in Yarn 4, so the very first setup step errors. Replace it with yarn install (or plain yarn) so readers can actually install dependencies.
- yarn i
+ yarn install📝 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.
| yarn i | |
| # Time: ~2.5 minutes | |
| # Timeout: Use 300+ seconds for bash commands | |
| ``` | |
| yarn install | |
| # Time: ~2.5 minutes | |
| # Timeout: Use 300+ seconds for bash commands |
🤖 Prompt for AI Agents
.github/copilot-instructions.md around lines 41 to 44: The install command uses
the unsupported alias "yarn i" which fails on Yarn 4; replace it with a
supported command such as "yarn install" or simply "yarn" in that line so
dependency installation works across Yarn versions.
|
|
||
| npmPublishAccess: public | ||
|
|
||
| yarnPath: .yarn/releases/yarn-4.10.3.cjs |
There was a problem hiding this comment.
Revert Yarn binary bump
The .yarnrc.yml coding guideline requires Yarn 4.9.1, but the yarnPath now points to 4.10.3. Please keep the pinned binary at 4.9.1 unless the guideline is updated. As per coding guidelines.
🤖 Prompt for AI Agents
In .yarnrc.yml around line 11, the yarnPath was updated to yarn-4.10.3 but our
coding guideline pins Yarn to 4.9.1; revert the yarnPath value to point to
.yarn/releases/yarn-4.9.1.cjs so the repo uses the approved binary version and
commit the change.
| ## 10.0.0-beta.9 | ||
|
|
||
| - Automigrations: Add automigration for viewport and backgrounds - [#31614](https://github.com/storybookjs/storybook/pull/31614), thanks @valentinpalkovic! | ||
| - Svelte: Simplify public types - use modern `Component` - [#31394](https://github.com/storybookjs/storybook/pull/31394), thanks @xeho91! | ||
| - Telemetry: Log userAgent in onboarding - [#32566](https://github.com/storybookjs/storybook/pull/32566), thanks @shilman! | ||
|
|
||
| ## 10.0.0-beta.8 | ||
|
|
||
| - Addon-docs: Add eject button to canvas toolbar - [#29825](https://github.com/storybookjs/storybook/pull/29825), thanks @mihkeleidast! | ||
| - Angular: Enable experimental zoneless detection on Angular v21 - [#32580](https://github.com/storybookjs/storybook/pull/32580), thanks @yannbf! | ||
| - AutoMigration: Fix sb10 migration when main config contains `require` - [#32558](https://github.com/storybookjs/storybook/pull/32558), thanks @ndelangen! | ||
| - Cleanup: Remove duplicated entrypoints in core - [#32507](https://github.com/storybookjs/storybook/pull/32507), thanks @ndelangen! | ||
| - Controls: Fix adding new values to arrays - [#32512](https://github.com/storybookjs/storybook/pull/32512), thanks @takashi-kasajima! | ||
| - Core: Fix `external-globals-plugin` handle `undefined` cache dir - [#32579](https://github.com/storybookjs/storybook/pull/32579), thanks @walkerburgin! | ||
| - Core: Use `exsolve` `resolveModulePath` for `safeResolveModule` - [#32477](https://github.com/storybookjs/storybook/pull/32477), thanks @mrginglymus! | ||
| - Next.js: Remove next/config usage in Next.js >=v16 projects - [#32547](https://github.com/storybookjs/storybook/pull/32547), thanks @valentinpalkovic! | ||
| - React Native: Fix document reference error in open-in-editor - [#32572](https://github.com/storybookjs/storybook/pull/32572), thanks @dannyhw! | ||
| - Svelte: Ignore inherited `HTMLAttributes` docgen when using utility types - [#32173](https://github.com/storybookjs/storybook/pull/32173), thanks @steciuk! | ||
| - UI: Improve sidebar empty state - [#32548](https://github.com/storybookjs/storybook/pull/32548), thanks @ghengeveld! | ||
|
|
There was a problem hiding this comment.
Align the release notes with this PR’s scope.
The bullets under 10.0.0-beta.9 / 10.0.0-beta.8 reference unrelated work (e.g., #31614, #29825) and never mention the Next.js + Vite RSC framework, routing mocks, or related changes you’re actually shipping here. That leaves the changelog misleading for anyone consuming the beta. Please rewrite these entries to capture the new framework/package additions from this PR (e.g., “Frameworks: add Next.js Vite RSC with RSC tooling – #32412”) and drop bullets that aren’t part of this release.
🤖 Prompt for AI Agents
In CHANGELOG.prerelease.md around lines 1 to 20, the release bullets under
10.0.0-beta.9 and 10.0.0-beta.8 list unrelated PRs and do not reflect the actual
scope of this PR; replace those unrelated bullets with concise entries that
capture this PR’s additions (e.g., Frameworks: add Next.js Vite RSC with RSC
tooling – #32412; Frameworks: add routing mocks for Next.js RSC – #32412;
Packages: add next-vite-rsc package and related exports – #32412), remove any
bullets not part of this release, keep the same markdown/bullet style, include
relevant PR numbers and contributor handles, and ensure the changelog accurately
describes the new framework/package features being shipped.
| - CSF: Story ComponentAnnotations['subcomponents'] to correctly use its own type for subcomponents rather than attempt to inherit from the component - [#31723](https://github.com/storybookjs/storybook/pull/31723), thanks @mihkeleidast! | ||
| - Controls: Improve the accessibility of the object control - [#31581](https://github.com/storybookjs/storybook/pull/31581), thanks @Sidnioulz! | ||
| - Core: Cleanup of type following up v9 and small verbatimModuleSyntax type fix - [#31823](https://github.com/storybookjs/storybook/pull/31823), thanks @alcpereira! | ||
| - Core: Cleanup of type following up v9 and small verbatimModuleSyntax type fix - [#31823](https://github.com/storybookjs/storybook/pull/31823), thanks @alcpereira! |
There was a problem hiding this comment.
Fix the list indentation.
There are four leading spaces before the bullet on Line 181, which markdownlint flags as MD007. Drop the extra indentation so the item aligns with the other top-level bullets.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
181-181: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🤖 Prompt for AI Agents
In CHANGELOG.prerelease.md around line 181, the list item has four leading
spaces causing MD007 (incorrect list indentation); remove the extra indentation
so the bullet aligns with the other top-level list items (i.e., start the line
with a single hyphen at the same column as other bullets) ensuring consistent
top-level bullet indentation.
| zoom={(z: number) => setScale(scale * z)} | ||
| resetZoom={() => setScale(1)} | ||
| storyId={getStoryId(children)} | ||
| storyId={!isLoading && childProps ? getStoryId(childProps, context) : undefined} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify parameter types for getStoryId call.
The conditional logic correctly guards against calling getStoryId during loading or when childProps is null. However, ensure that getStoryId can safely accept the return type from getChildProps (Record<string, any> | null).
Run the following script to verify the getStoryId function signature:
🏁 Script executed:
#!/bin/bash
# Description: Verify getStoryId function signature and parameter expectations
# Search for getStoryId function definition
ast-grep --pattern $'export function getStoryId($$$) {
$$$
}'
# Also search for getStoryId type definition
rg -nP --type=ts -A5 'function getStoryId|type.*getStoryId'Length of output: 2311
Fix getStoryId invocation
getStoryId expects (data: StoryIdData, options: Options), but you’re passing a generic childProps and the DocsContext object. Ensure childProps conforms to StoryIdData and supply a valid Options object (presets/configDir) instead of DocsContext.
🤖 Prompt for AI Agents
In code/addons/docs/src/blocks/components/Preview.tsx around line 262, the call
to getStoryId currently passes a generic childProps and the entire DocsContext;
update the invocation so childProps is asserted/structured as StoryIdData and
pass a proper Options object (containing the presets/configDir values) instead
of the DocsContext. Concretely, ensure childProps implements the StoryIdData
shape (pick/transform the needed fields or cast it) and construct an options
object from the available presets/configDir (or import defaults) and pass that
as the second argument to getStoryId.
| const included = Object.values( | ||
| groupByType(Array.from(includedFilters).map((id) => filtersById[id])) | ||
| ); | ||
| const excluded = Object.values( | ||
| groupByType(Array.from(excludedFilters).map((id) => filtersById[id])) | ||
| ); | ||
|
|
||
| return ( | ||
| (!included.length || | ||
| included.every((group) => group.some(({ filterFn }) => filterFn(item, false)))) && | ||
| (!excluded.length || | ||
| excluded.every((group) => group.every(({ filterFn }) => filterFn(item, true)))) | ||
| ); |
There was a problem hiding this comment.
Guard against missing filters before grouping.
If a previously selected tag disappears from filtersById (e.g., story index refresh without that tag), filtersById[id] returns undefined. Passing that into groupByType causes filter.type access on undefined, crashing the manager once the effect re-runs. Filter out missing entries (or clean stale IDs) before grouping so we never dereference undefined.
🤖 Prompt for AI Agents
In code/core/src/manager/components/sidebar/TagsFilter.tsx around lines 148 to
160, selected filter IDs may map to undefined when filtersById no longer
contains a tag; passing undefined into groupByType causes a crash when accessing
filter.type. Before calling groupByType, filter the mapped arrays to remove
falsy/undefined entries (or alternatively prune stale IDs from
includedFilters/excludedFilters), e.g. map ids to filtersById[id], then drop
undefined results, then pass the cleaned array to groupByType so groupByType
never receives undefined.
| while (queuedErrors.length > 0) { | ||
| const queuedError = queuedErrors.shift(); | ||
| channel.emit(TELEMETRY_ERROR, queuedError); |
There was a problem hiding this comment.
Guard against shift() returning undefined.
Inside the flush loop, queuedErrors.shift() has the type Error | undefined, so passing it straight to channel.emit violates strict null checks and breaks compilation. Please add a guard (or use non-null assertion once the guard runs) before emitting.
- while (queuedErrors.length > 0) {
- const queuedError = queuedErrors.shift();
- channel.emit(TELEMETRY_ERROR, queuedError);
- }
+ while (queuedErrors.length > 0) {
+ const queuedError = queuedErrors.shift();
+ if (queuedError) {
+ channel.emit(TELEMETRY_ERROR, queuedError);
+ }
+ }📝 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.
| while (queuedErrors.length > 0) { | |
| const queuedError = queuedErrors.shift(); | |
| channel.emit(TELEMETRY_ERROR, queuedError); | |
| while (queuedErrors.length > 0) { | |
| const queuedError = queuedErrors.shift(); | |
| if (queuedError) { | |
| channel.emit(TELEMETRY_ERROR, queuedError); | |
| } | |
| } |
🤖 Prompt for AI Agents
In code/core/src/manager/globals-runtime.ts around lines 29-31, the call to
queuedErrors.shift() can return undefined and is passed directly to
channel.emit, breaking strict null checks; change the loop to capture the result
in a variable, guard that it is not undefined before calling channel.emit (or
check length and use a non-null assertion after confirming the item is present),
ensuring you only emit when queuedError is defined.
| if (duplicatedKeys.includes(key)) { | ||
| acc[key.replace('storybook', 'storybook/internal') as typeof key] = { | ||
| type: 'esm', | ||
| varName: globalsNameReferenceMap[key], | ||
| namedExports: Exports[key], | ||
| defaultExport: true, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify type safety and existence of duplicated keys in globalsNameReferenceMap.
The type cast as typeof key assumes that the duplicated key (e.g., 'storybook/internal/theming') exists in globalsNameReferenceMap, but this is not enforced by the type system. If a duplicated key is not present in globalsNameReferenceMap, the type assertion is unsafe and could lead to runtime issues or incorrect bundling behavior.
Additionally, using the same varName for both the original and duplicated keys means two different import paths will resolve to the same global variable. Please confirm this is intentional.
Run the following script to verify that all duplicated keys exist in globalsNameReferenceMap:
🏁 Script executed:
#!/bin/bash
# Description: Verify that all duplicated internal keys exist in globalsNameReferenceMap.
echo "=== Checking duplicated keys in globalsNameReferenceMap ==="
# Extract duplicatedKeys array from this file
duplicated_keys=$(cat code/core/src/manager/globals/globals-module-info.ts | sed -n '/const duplicatedKeys = \[/,/\];/p' | grep "'" | sed "s/.*'\(.*\)'.*/\1/")
echo "Duplicated keys:"
echo "$duplicated_keys"
echo ""
# Check if the corresponding 'storybook/internal/*' versions exist in globalsNameReferenceMap
echo "Checking if 'storybook/internal/*' versions exist in globalsNameReferenceMap:"
for key in $duplicated_keys; do
internal_key=$(echo "$key" | sed 's/storybook/storybook\/internal/')
echo -n " $internal_key: "
if rg -q "'$internal_key'" code/core/src/manager/globals/globals.ts code/core/src/preview/globals/globals.ts; then
echo "✓ Found"
else
echo "✗ NOT FOUND (type assertion may be unsafe)"
fi
doneLength of output: 814
Ensure duplicated internal keys exist or remove duplication logic
All generated 'storybook/internal/*' keys from duplicatedKeys (theming, theming/create, manager-api, test, actions, highlight, viewport) are not present in globalsNameReferenceMap, making the as typeof key assertion unsafe and leading to missing globals at runtime. Add these internal entries or remove this duplication block.
🤖 Prompt for AI Agents
In code/core/src/manager/globals/globals-module-info.ts around lines 44 to 51,
the duplication branch generates 'storybook/internal/*' keys that are not
present in globalsNameReferenceMap which makes the `as typeof key` cast unsafe
and causes missing globals at runtime; either (A) add the corresponding
'storybook/internal/...' entries to globalsNameReferenceMap mapping each
internal key to the same variable name (and ensure Exports has the same
namedExports/defaultExport values) so the duplication block can safely set
type/varName/namedExports/defaultExport, or (B) remove the duplication block
entirely if internal keys are unnecessary — pick one approach and update
tests/consumers to ensure no runtime missing-global errors.
| // When react-dev-tools is installed as a browser extension, it will log this error | ||
| 'React is running in production mode', |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify the exact error message text.
The error message 'React is running in production mode' may not match what React DevTools actually logs. React DevTools typically emits warnings about production mode (e.g., "Download the React DevTools for a better development experience"), not errors with this exact text.
Please run the following script to search for any existing references to this error message in the codebase or related documentation:
Additionally, please search the web for the exact error message to confirm its accuracy:
🏁 Script executed:
#!/bin/bash
# Description: Search for references to the error message in the codebase and verify its accuracy.
# Search for the error message text
rg -n "React is running in production mode" --type ts --type tsx --type js --type jsx -C 3
# Search for related React DevTools error patterns
rg -n "React.*production.*mode|DevTools.*production" --type ts --type tsx --type js --type jsx -C 3Length of output: 55
🌐 Web query:
What is the exact error message logged by React DevTools when running in production mode?
💡 Result:
The message is:
Download the React DevTools for a better development experience: https://reactjs.org/link/react-devtools. [1][2]
Sources:
[1] Stack Overflow — "Disable/Hide 'Download the React DevTools...'" discussion.
[2] GitHub issue referencing the same console output.
Update telemetry exclusion to actual DevTools message Replace 'React is running in production mode' (prepareForTelemetry.ts:28) with 'Download the React DevTools for a better development experience: https://reactjs.org/link/react-devtools'.
🤖 Prompt for AI Agents
In code/core/src/manager/utils/prepareForTelemetry.ts around lines 28-29, the
telemetry exclusion currently filters the wrong message ('React is running in
production mode'); replace that entry with the exact DevTools message 'Download
the React DevTools for a better development experience:
https://reactjs.org/link/react-devtools' so telemetry correctly ignores the
React DevTools console output.
| placeholder?: string; | ||
| initialValue?: string; | ||
| validate?: (value: string) => string | Error | undefined; | ||
| validate?: (value: string | undefined) => string | Error | undefined; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify existing validators handle undefined input.
The validate signature now accepts string | undefined instead of just string. This is a potentially breaking change—any existing validators that don't explicitly check for undefined may throw runtime errors or behave unexpectedly.
Run the following script to locate all implementations and usages of TextPromptOptions validators:
Ensure all validators handle undefined gracefully (e.g., early return or default behavior when value is undefined).
🏁 Script executed:
#!/bin/bash
# Description: Find all implementations of TextPromptOptions and usage of validate property.
# Find implementations of PromptProvider.text method
echo "=== PromptProvider.text implementations ==="
ast-grep --pattern $'class $_ {
$$$
text($$$) {
$$$
}
$$$
}'
# Find all usages of TextPromptOptions with validate property
echo -e "\n=== TextPromptOptions with validate property ==="
rg -nP --type=ts -C3 'validate\s*:\s*\('
# Find calls to text() method that may pass undefined
echo -e "\n=== Calls to text() method ==="
rg -nP --type=ts -C3 '\.text\s*\('Length of output: 10042
Handle undefined in TextPromptOptions.validate implementations
The validate callback now accepts string | undefined. Existing validators must explicitly handle undefined to avoid runtime or type errors. Update implementations in:
- code/lib/cli-storybook/src/sandbox.ts
- code/lib/eslint-plugin/scripts/generate-rule.ts
🤖 Prompt for AI Agents
In code/core/src/node-logger/prompts/prompt-provider-base.ts around line 22 the
TextPromptOptions.validate signature was widened to accept string | undefined;
update the validate implementations in code/lib/cli-storybook/src/sandbox.ts and
code/lib/eslint-plugin/scripts/generate-rule.ts to explicitly handle undefined
inputs (e.g., treat undefined as '' or return a validation Error/string
indicating a required value) and ensure they return string | Error | undefined
accordingly; adjust any branches that assume value is a string (trim/length
checks, regex tests) to guard for undefined before operating on the value.
|
Failed to publish canary version of this pull request, triggered by @shilman. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/18215023370 |
kasperpeulen
changed the base branch from
kasper/nextjs-vite-rsc-base
to
kasper/nextjs-vite-rsc-base-2
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/core/src/common/versions.ts (1)
1-1: Add@storybook/nextjs-vite-rscto the generator, not toversions.tsdirectly
The header “auto generated file, do not edit” shows this file is maintained by scripts/release/version.ts. Manual edits here will be overwritten on the next bump. Update the generator’s package list (in scripts/release/version.ts or its source) to include the new entry so it’s emitted automatically.
♻️ Duplicate comments (4)
code/frameworks/nextjs-vite-rsc/src/preview.tsx (4)
10-10: Deep import from Next internals remains.The import from
next/dist/client/components/is-next-router-erroris still present and is not a public API. This was flagged in a previous review and remains unaddressed.
76-78: Critical: loaders must be an array.The
loadersexport is typed and defined as a singleLoaderFunction, but Storybook's preview expectsloadersto be an array of loader functions. This was flagged in a previous review and remains unaddressed.
114-119: Critical: Story function mis-invoked in applyDecorators.
storyFnis aLegacyStoryFnand should be invoked as a function withcontext, not mounted as a React component viaReact.createElement(storyFn, context). This passescontextas props instead of invoking the story function, breaking args and decorator behavior. This was flagged in a previous review and remains unaddressed.
121-136: Critical: Story function mis-invoked in renderToCanvas.
unboundStoryFnis a story function and should be invoked withstoryContextas an argument, not mounted as a React component via<Story {...storyContext} />. This passesstoryContextas props instead of invoking the story function, breaking the story rendering flow. This was flagged in a previous review and remains unaddressed.
🧹 Nitpick comments (2)
code/frameworks/nextjs-vite-rsc/src/preview.tsx (1)
64-69: Expand decorator to support full navigation parameters.The decorator now reads
context.parameters?.nextjs?.pathname, which is an improvement. However, it only passespathnameasurland doesn't support other navigation properties likesearchorqueryparameters.Consider constructing the full URL from navigation parameters:
export const decorators: Decorator[] = [ - (Story, context) => ( - <NextRouter url={context.parameters?.nextjs?.pathname ?? ''}> - <Story /> - </NextRouter> - ), + (Story, context) => { + const nav = context.parameters?.nextjs?.navigation ?? {}; + const pathname = nav.pathname ?? context.parameters?.nextjs?.pathname ?? '/'; + const search = nav.search ?? ''; + const url = nav.url ?? `${pathname}${search}`; + return ( + <NextRouter url={url} route={nav.route ?? pathname}> + <Story /> + </NextRouter> + ); + },scripts/build/utils/entry-utils.ts (1)
74-79: Consider grouping RSC externals with a comment.For improved maintainability, consider grouping these five RSC-related externals together with a descriptive comment, similar to the existing 'sb-original' entry pattern.
- // TODO where to place this '@storybook/nextjs-vite-rsc/rsc/client', '@storybook/nextjs-vite-rsc/preview', + // Virtual modules for Next.js RSC browser-mode support 'virtual:vite-rsc-browser-mode/load-client', 'virtual:vite-rsc-browser-mode/build-server-references', 'virtual:vite-rsc-browser-mode/build-client-references'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
code/core/src/common/versions.ts(1 hunks)code/frameworks/nextjs-vite-rsc/package.json(4 hunks)code/frameworks/nextjs-vite-rsc/src/preview.tsx(3 hunks)code/frameworks/nextjs-vite-rsc/src/rsc-plugin.ts(1 hunks)scripts/build/utils/entry-utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- code/frameworks/nextjs-vite-rsc/package.json
- code/frameworks/nextjs-vite-rsc/src/rsc-plugin.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/core/src/common/versions.tscode/frameworks/nextjs-vite-rsc/src/preview.tsxscripts/build/utils/entry-utils.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/core/src/common/versions.tscode/frameworks/nextjs-vite-rsc/src/preview.tsxscripts/build/utils/entry-utils.ts
🧠 Learnings (2)
📚 Learning: 2025-09-24T09:39:39.233Z
Learnt from: ndelangen
PR: storybookjs/storybook#32507
File: code/core/src/manager/globals/globals-module-info.ts:25-33
Timestamp: 2025-09-24T09:39:39.233Z
Learning: In Storybook, storybook/actions/decorator is a preview-only entrypoint and should not be included in manager globals configuration. The duplicatedKeys array in code/core/src/manager/globals/globals-module-info.ts is specifically for manager-side externalization, not preview entrypoints.
Applied to files:
code/frameworks/nextjs-vite-rsc/src/preview.tsx
📚 Learning: 2025-09-17T07:31:04.432Z
Learnt from: ndelangen
PR: storybookjs/storybook#32484
File: code/core/package.json:326-326
Timestamp: 2025-09-17T07:31:04.432Z
Learning: In Storybook's core package, dependencies like `open` are bundled into the final distribution during the build process, so they should remain in devDependencies rather than being moved to dependencies. End users don't need these packages as separate runtime dependencies since they're included in the bundled code.
Applied to files:
scripts/build/utils/entry-utils.ts
🧬 Code graph analysis (1)
code/frameworks/nextjs-vite-rsc/src/preview.tsx (2)
code/frameworks/nextjs-vite-rsc/src/rsc/client.tsx (1)
NextRouter(31-72)code/frameworks/nextjs-vite-rsc/src/testing-library.tsx (2)
initialize(107-125)renderServer(17-80)
⏰ 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: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (3)
code/frameworks/nextjs-vite-rsc/src/preview.tsx (1)
22-27: LGTM: Global onNavigate mock is now initialized.The explicit initialization of
globalThis.onNavigatewith a mock function at module load time addresses the previous concern about undefined errors during navigation.code/core/src/common/versions.ts (1)
19-19: LGTM! New framework package version entry is consistent.The addition of
@storybook/nextjs-vite-rscfollows the existing pattern with correct alphabetical ordering and matching version number.scripts/build/utils/entry-utils.ts (1)
74-79: Remove the TODO comment in scripts/build/utils/entry-utils.ts. The new externals belong in runtimeExternalInclude and match their declarations in rsc-plugin.ts. Optional: add a brief grouping comment above these entries for clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
code/core/src/csf/csf-factories.ts (2)
194-196: PasstestNamethrough when invokingcompose().runThe
testNameparameter advertised in the interface (line 142) is not forwarded to the composed story's runner, preventing callers from targeting specific tests.Apply this diff:
- async run(context) { - await compose().run(context); + async run(context, testName) { + await compose().run(context, testName); },
225-225: Stop returning a value from avoid-typed methodThe
testmethod is declaredvoid(lines 148-153), yet it returns the test instance. This violates thenoVoidTypeReturnlint rule and will fail CI.Apply this diff:
__children.push(test); - - return test as unknown as void; + },
🧹 Nitpick comments (2)
code/core/src/preview-api/modules/preview-web/render/mount-utils.ts (1)
3-4: Remove unused imports.
PlayFunctionandRendererare imported but no longer used in the function signature after the type change.Apply this diff to remove unused imports:
-import type { PlayFunction } from 'storybook/internal/csf'; -import { type Renderer } from 'storybook/internal/types'; - export function mountDestructured(playFunction?: (...args: any[]) => any): boolean {code/core/src/csf/csf-factories.ts (1)
207-208: Add explanatory comment formountdestructuring. Themountparameter is destructured solely to satisfymountDestructureddetection in the play function; add an inline comment above this async function to explain why it’s unused.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/core/src/csf/csf-factories.ts(1 hunks)code/core/src/preview-api/modules/preview-web/render/mount-utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/core/src/csf/csf-factories.tscode/core/src/preview-api/modules/preview-web/render/mount-utils.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/core/src/csf/csf-factories.tscode/core/src/preview-api/modules/preview-web/render/mount-utils.ts
🧬 Code graph analysis (1)
code/core/src/csf/csf-factories.ts (1)
code/core/src/csf/story.ts (1)
StoryContext(252-266)
⏰ 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: normal
- GitHub Check: Core Unit Tests, windows-latest
| export function mountDestructured<TRenderer extends Renderer>( | ||
| playFunction?: PlayFunction<TRenderer> | ||
| ): boolean { | ||
| export function mountDestructured(playFunction?: (...args: any[]) => any): boolean { |
There was a problem hiding this comment.
Type safety regression: prefer precise typings over any.
The function signature changed from PlayFunction<TRenderer> to (...args: any[]) => any, eliminating type safety. This violates the coding guideline to "prefer precise typings instead of using any or suppressions, consistent with strict mode."
If framework flexibility is required, consider alternatives that preserve type safety:
- Use a union of known function types
- Introduce a base type that captures common structure
- Use generics with constraints that accommodate the new frameworks
As per coding guidelines.
🤖 Prompt for AI Agents
In code/core/src/preview-api/modules/preview-web/render/mount-utils.ts around
line 6, the mountDestructured parameter was changed to a loose (...args: any[])
=> any which loses type safety; restore precise typing by reverting to the
original PlayFunction<TRenderer> signature or introduce a generic constraint
(e.g., <TRenderer> and accept playFunction?: PlayFunction<TRenderer> or a
constrained union/base function type) so callers keep strict-mode type
guarantees; update the function signature and any related generics/imports
accordingly, avoiding use of any.
|
Failed to publish canary version of this pull request, triggered by @kasperpeulen. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/18315280782 |
Closes #
What I did
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This pull request has been released as version
0.0.0-pr-32412-sha-b8362176. Try it out in a new sandbox by runningnpx [email protected] sandboxor in an existing project withnpx [email protected] upgrade.More information
0.0.0-pr-32412-sha-b8362176kasper/nextjs-vite-rscb83621761759850758)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=32412Greptile Summary
Updated On: 2025-09-04 12:19:39 UTC
This PR introduces a new Next.js framework variant called
nextjs-vite-rscthat combines Next.js with Vite bundling and React Server Components (RSC) support. The new framework provides a comprehensive implementation including:Core Framework Structure:
Mock System Implementation:
Template Files and Examples:
Integration Features:
The framework follows the same architectural patterns as existing Next.js frameworks (
nextjsandnextjs-vite) but is specifically designed for React Server Components, which require different handling for server-side rendering, component compilation, and runtime behavior. This separation allows for RSC-specific optimizations without impacting existing framework implementations.PR Description Notes:
Confidence score: 2/5
Context used:
Context - PR titles must be in the format of 'Area: Summary', with both Area and Summary starting with a capital letter. (link)
Summary by CodeRabbit
New Features
Templates
Chores
Tooling