Maintenance: extract shared Next.js preview setup to eliminate nextjs/nextjs-vite preview.tsx duplication#34475
Closed
mixelburg wants to merge 1 commit into
Closed
Conversation
Contributor
📝 WalkthroughWalkthroughRefactored Next.js Storybook integration by extracting error handling utilities from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/frameworks/nextjs/src/preview-shared.ts (1)
3-39: Make global patch setup idempotent.
setupNextErrorPatching()currently re-patchesconsole.error, re-adds the globalerrorlistener, and re-appendsnext-head-counton every invocation. A one-time guard avoids stacked wrappers/listeners during re-import/HMR.Proposed hardening
import { isNextRouterError } from 'next/dist/client/components/is-next-router-error.js'; +let nextErrorPatchingInstalled = false; + export function addNextHeadCount() { + if (document.head.querySelector('meta[name="next-head-count"]')) { + return; + } const meta = document.createElement('meta'); meta.name = 'next-head-count'; meta.content = '0'; document.head.appendChild(meta); } @@ export function setupNextErrorPatching() { + if (nextErrorPatchingInstalled) { + return; + } + nextErrorPatchingInstalled = true; + addNextHeadCount(); @@ globalThis.addEventListener('error', (ev: WindowEventMap['error']): void => { if (isNextRouterError(ev.error) || isAsyncClientComponentError(ev.error)) { ev.preventDefault(); return; } }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/frameworks/nextjs/src/preview-shared.ts` around lines 3 - 39, The setupNextErrorPatching function should be made idempotent: add a one-time guard (e.g., a module-level flag or a symbol on globalThis such as globalThis.__nextErrorPatchingDone) checked at the top of setupNextErrorPatching to return early if already applied; modify addNextHeadCount to no-op if a meta[name="next-head-count"] already exists; only wrap globalThis.console.error if it hasn't been wrapped yet (detect via the guard or by checking a marker on the function) and only add the globalThis.addEventListener('error', ...) listener once (store the listener reference or rely on the same guard to avoid re-adding). Ensure the guard is set after successfully applying the patches so repeated calls to setupNextErrorPatching or HMR re-imports do nothing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/frameworks/nextjs/package.json`:
- Around line 74-78: The package export for "./preview-shared" is declared in
package.json but no build entry is configured to produce dist/preview-shared.*;
update build-config.ts to add an export entry that builds src/preview-shared.ts
by adding exportEntries: ['./preview-shared'] (or include
'./src/preview-shared.ts' as a build entry) so the bundler emits
dist/preview-shared.js and dist/preview-shared.d.ts; locate the configuration in
build-config.ts and add the new entry alongside the other Storybook/Next.js
entries to ensure `@storybook/nextjs/preview-shared` imports resolve.
---
Nitpick comments:
In `@code/frameworks/nextjs/src/preview-shared.ts`:
- Around line 3-39: The setupNextErrorPatching function should be made
idempotent: add a one-time guard (e.g., a module-level flag or a symbol on
globalThis such as globalThis.__nextErrorPatchingDone) checked at the top of
setupNextErrorPatching to return early if already applied; modify
addNextHeadCount to no-op if a meta[name="next-head-count"] already exists; only
wrap globalThis.console.error if it hasn't been wrapped yet (detect via the
guard or by checking a marker on the function) and only add the
globalThis.addEventListener('error', ...) listener once (store the listener
reference or rely on the same guard to avoid re-adding). Ensure the guard is set
after successfully applying the patches so repeated calls to
setupNextErrorPatching or HMR re-imports do nothing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 411166f4-bc65-4449-a7f8-c6a8e1024076
📒 Files selected for processing (5)
code/frameworks/nextjs-vite/package.jsoncode/frameworks/nextjs-vite/src/preview.tsxcode/frameworks/nextjs/package.jsoncode/frameworks/nextjs/src/preview-shared.tscode/frameworks/nextjs/src/preview.tsx
Comment on lines
+74
to
+78
| "./preview-shared": { | ||
| "types": "./dist/preview-shared.d.ts", | ||
| "code": "./src/preview-shared.ts", | ||
| "default": "./dist/preview-shared.js" | ||
| }, |
Contributor
There was a problem hiding this comment.
./preview-shared export targets are currently not build-produced.
Lines 74-78 expose ./dist/preview-shared.*, but code/frameworks/nextjs/build-config.ts does not define an entry for ./src/preview-shared.ts with exportEntries: ['./preview-shared']. This can ship a broken export and break imports such as @storybook/nextjs/preview-shared.
Proposed fix (add missing build entry)
diff --git a/code/frameworks/nextjs/build-config.ts b/code/frameworks/nextjs/build-config.ts
@@
{
exportEntries: ['./preview'],
entryPoint: './src/preview.tsx',
},
+ {
+ exportEntries: ['./preview-shared'],
+ entryPoint: './src/preview-shared.ts',
+ },
{
exportEntries: ['./config/preview'],
entryPoint: './src/config/preview.ts',
dts: false,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "./preview-shared": { | |
| "types": "./dist/preview-shared.d.ts", | |
| "code": "./src/preview-shared.ts", | |
| "default": "./dist/preview-shared.js" | |
| }, | |
| { | |
| exportEntries: ['./preview'], | |
| entryPoint: './src/preview.tsx', | |
| }, | |
| { | |
| exportEntries: ['./preview-shared'], | |
| entryPoint: './src/preview-shared.ts', | |
| }, | |
| { | |
| exportEntries: ['./config/preview'], | |
| entryPoint: './src/config/preview.ts', | |
| dts: false, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/frameworks/nextjs/package.json` around lines 74 - 78, The package export
for "./preview-shared" is declared in package.json but no build entry is
configured to produce dist/preview-shared.*; update build-config.ts to add an
export entry that builds src/preview-shared.ts by adding exportEntries:
['./preview-shared'] (or include './src/preview-shared.ts' as a build entry) so
the bundler emits dist/preview-shared.js and dist/preview-shared.d.ts; locate
the configuration in build-config.ts and add the new entry alongside the other
Storybook/Next.js entries to ensure `@storybook/nextjs/preview-shared` imports
resolve.
Contributor
|
Closing due to #34453 (comment) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #34465
What I did
code/frameworks/nextjs/src/preview.tsxandcode/frameworks/nextjs-vite/src/preview.tsxcontained ~55 lines of near-identical code:addNextHeadCount(),isAsyncClientComponentError(), the globalconsole.errorpatch, theerrorevent listener override, and the entireparametersexport.Created
code/frameworks/nextjs/src/preview-shared.tswith:addNextHeadCount()isAsyncClientComponentError()setupNextErrorPatching()— wraps the console.error patch + error event listenerparametersexportBoth
preview.tsxfiles now import these from the shared module and callsetupNextErrorPatching()at module load time, preserving the original execution order.Files changed:
code/frameworks/nextjs/src/preview-shared.ts— shared utilitiescode/frameworks/nextjs/src/preview.tsx— imports from shared modulecode/frameworks/nextjs-vite/src/preview.tsx— imports from@storybook/nextjs/preview-shared./preview-sharedexport entry to@storybook/nextjspackage.json@storybook/nextjs: workspace:*to@storybook/nextjs-vitedependenciesThe decorators and loaders sections remain intentionally different between the two files (different type signatures for vite vs webpack).
Note: This PR adds
@storybook/nextjsas a dependency of@storybook/nextjs-vite— the same dependency already added in #34474 for theportable-storiesdedup. If that PR merges first, thepackage.jsonchange here will be a no-op.Checklist for Contributors
Testing
Manual testing
Pure refactor — no behavior changes. Both files call
setupNextErrorPatching()once at module load time, same as the inline code before.Summary by CodeRabbit