Addon-docs: Fix source panel disabling #29791
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 80f375d. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
14 file(s) reviewed, 12 comment(s)
Edit PR Review Bot Settings | Greptile
| @@ -1,5 +1,7 @@ | |||
| <h1>Migration</h1> | |||
|
|
|||
| - [From version 8.4.x to 8.5.x](#from-version-84x-to-85x) | |||
| - [Added source code pnael to docs](#added-source-code-pnael-to-docs) | |||
There was a problem hiding this comment.
syntax: 'pnael' is misspelled, should be 'panel'
| @@ -0,0 +1,15 @@ | |||
| export default { | |||
| component: globalThis.Components.Button, | |||
There was a problem hiding this comment.
logic: globalThis.Components.Button may not be available in all environments - should verify this component exists or use a more reliable import
| @@ -129,7 +129,7 @@ export const TestProviderRender: FC< | |||
| padding="small" | |||
| active={state.watching} | |||
| onClick={() => api.setTestProviderWatchMode(state.id, !state.watching)} | |||
| disabled={state.crashed || state.running || isEditing} | |||
| disabled={state.running || isEditing} | |||
There was a problem hiding this comment.
logic: removing crashed state check could allow re-running tests in an unstable state
| @@ -152,7 +152,7 @@ export const TestProviderRender: FC< | |||
| variant="ghost" | |||
| padding="small" | |||
| onClick={() => api.runTestProvider(state.id, { entryId })} | |||
| disabled={state.crashed || state.running || isEditing} | |||
| disabled={state.running || isEditing} | |||
There was a problem hiding this comment.
logic: removing crashed state check here could lead to test runner instability if tests are re-run after a crash without cleanup
| @@ -0,0 +1,56 @@ | |||
| import { beforeEach, describe, expect, it, vi } from 'vitest'; | |||
There was a problem hiding this comment.
style: beforeEach and vi are imported but never used in the test file
| export async function toImportFn(root: string, stories: string[]) { | ||
| const objectEntries = stories.map((file) => { | ||
| const relativePath = normalizePath(relative(process.cwd(), file)); | ||
| const relativePath = relative(root, file); | ||
|
|
||
| return ` '${toImportPath(relativePath)}': async () => import('/@fs/${file}')`; | ||
| return [toImportPath(relativePath), genDynamicImport(normalize(file))] as [string, string]; | ||
| }); |
There was a problem hiding this comment.
logic: normalize() should be called before relative() to ensure consistent path separators across platforms
| // eslint-disable-next-line @typescript-eslint/return-await | ||
| return await toImportFn(options.projectRoot || process.cwd(), stories); |
There was a problem hiding this comment.
style: unnecessary return-await since toImportFn already returns a Promise
| @@ -53,18 +53,18 @@ export async function commonConfig( | |||
|
|
|||
| const { viteConfigPath } = await getBuilderOptions<BuilderOptions>(options); | |||
|
|
|||
| const projectRoot = resolve(options.configDir, '..'); | |||
| options.projectRoot = options.projectRoot || resolve(options.configDir, '..'); | |||
There was a problem hiding this comment.
logic: This mutates the options object directly, which could cause side effects. Consider creating a new object or using a local variable instead.
| if (!main._exportsObject) { | ||
| // eslint-disable-next-line local-rules/no-uncategorized-errors | ||
| throw new Error( | ||
| `Unable to parse Storybook main file while trying to read 'core' property` | ||
| ); | ||
| } |
There was a problem hiding this comment.
logic: This error check should come before using main.core on line 63-64, since the same condition could cause that to fail
| "targets": { | ||
| "sandbox": {}, | ||
| "sb:dev": {}, | ||
| "sb:build": {} | ||
| }, |
There was a problem hiding this comment.
style: empty target configurations may need to be filled in with actual build/dev settings
…urce-panel-parameter
shilman
merged commit 88c64d8
into
larsrickert/1898-story-code-panel
What I did
Use built-in method for disabling the new source panel.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Codepanel🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>Greptile Summary
Based on the provided information, I'll create a concise summary of the pull request focusing on the key changes and their implications.
Simplified source panel disabling in Storybook Docs addon, focusing on using built-in parameter methods for configuration.
docsSourcePanelparameter to control source panel visibility globally or per-storyparamKeysourcePanel/index.stories.tsxThis PR represents a significant improvement in how the source panel can be disabled, making it more consistent with Storybook's standard parameter handling patterns. The changes are well-documented and include demonstration stories, though additional test coverage could be beneficial.