Improve type of GlobalTypes to add typed toolbar option#33267
Improve type of GlobalTypes to add typed toolbar option#33267mrginglymus wants to merge 2 commits into
Conversation
|
View your CI Pipeline Execution ↗ for commit f3d4cbc ☁️ Nx Cloud last updated this comment at |
mrginglymus
force-pushed
the
global-types-toolbar-type
branch
from
3dd80fb to
a6bc25e
Compare
| [key: string]: any; | ||
| } | ||
|
|
||
| export type { ToolbarArgType }; |
There was a problem hiding this comment.
Not sure about where this should be exported from, but it should be exported.
| } | ||
| export interface StrictGlobalTypes { | ||
| [name: string]: StrictInputType; | ||
| [name: string]: StrictInputType & ToolbarArgType; |
There was a problem hiding this comment.
I'm not particularly happy about this one either to be honest. Should GlobalTypes even be using InputType as its bas? How much of InputType does it actually use? defaultValue is marked as deprecated in InputType but is used in ToolbarArgType. Perhaps we should decouple these types?
ndelangen
added
bug
core
typescript
preview-api
argtypes
ci:normal
and removed
Stale
labels
📝 WalkthroughWalkthroughExposes Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes ✨ Finishing touches
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: 2
♻️ Duplicate comments (3)
code/core/src/manager-api/tests/globals.test.ts (1)
14-14: Same type assertion pattern as other test files.Similar to
GlobalsStore.test.ts, this file uses type assertionsas ToolbarArgType. The same verification recommendations apply here to ensure these casts are appropriate and not masking type incompatibilities.Also applies to: 65-65, 92-92, 142-142
code/renderers/svelte/src/__test__/composeStories/portable-stories.test.ts (1)
75-75: Type assertion follows test pattern.The type assertion here is consistent with other test files in the PR. Same verification recommendations apply.
code/core/src/preview-api/modules/store/csf/portable-stories.test.ts (1)
11-11: Consistent test pattern with type assertions.This file follows the same pattern as other test files with type assertions for
ToolbarArgType. The internal import path is appropriate for core modules.Also applies to: 106-106, 239-239
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
code/core/src/csf/story.tscode/core/src/manager-api/tests/globals.test.tscode/core/src/preview-api/modules/store/GlobalsStore.test.tscode/core/src/preview-api/modules/store/csf/normalizeProjectAnnotations.tscode/core/src/preview-api/modules/store/csf/portable-stories.test.tscode/core/src/types/modules/csf.tscode/renderers/svelte/src/__test__/composeStories/portable-stories.test.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{test,spec}.{ts,tsx}: Test files should follow the naming pattern*.test.ts,*.test.tsx,*.spec.ts, or*.spec.tsx
Follow the spy mocking rules defined in.cursor/rules/spy-mocking.mdcfor consistent mocking patterns with Vitest
Files:
code/renderers/svelte/src/__test__/composeStories/portable-stories.test.tscode/core/src/manager-api/tests/globals.test.tscode/core/src/preview-api/modules/store/csf/portable-stories.test.tscode/core/src/preview-api/modules/store/GlobalsStore.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Usevi.mock()with thespy: trueoption for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Usevi.mocked()to type and access the mocked functions in Vitest tests
Implement mock behaviors inbeforeEachblocks in Vitest tests
Mock all required dependencies that the test subject uses
Each mock implementation should return a Promise for async functions in Vitest
Mock implementations should match the expected return type of the original function
Mock all required properties and methods that the test subject uses in Vitest tests
Avoid direct function mocking withoutvi.mocked()in Vitest tests
Avoid mock implementations outside ofbeforeEachblocks in Vitest tests
Avoid mocking without thespy: trueoption in Vitest tests
Avoid inline mock implementations within test cases in Vitest tests
Avoid mocking only a subset of required dependencies in Vitest tests
Mock at the highest level of abstraction needed in Vitest tests
Keep mock implementations simple and focused in Vitest tests
Use type-safe mocking withvi.mocked()in Vitest tests
Document complex mock behaviors in Vitest tests
Group related mocks together in Vitest tests
Files:
code/renderers/svelte/src/__test__/composeStories/portable-stories.test.tscode/core/src/manager-api/tests/globals.test.tscode/core/src/preview-api/modules/store/csf/portable-stories.test.tscode/core/src/preview-api/modules/store/GlobalsStore.test.ts
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with
yarn prettier --write <file>
Files:
code/renderers/svelte/src/__test__/composeStories/portable-stories.test.tscode/core/src/manager-api/tests/globals.test.tscode/core/src/preview-api/modules/store/csf/portable-stories.test.tscode/core/src/preview-api/modules/store/csf/normalizeProjectAnnotations.tscode/core/src/csf/story.tscode/core/src/types/modules/csf.tscode/core/src/preview-api/modules/store/GlobalsStore.test.ts
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run ESLint checks using
yarn lint:js:cmd <file>or the full commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/renderers/svelte/src/__test__/composeStories/portable-stories.test.tscode/core/src/manager-api/tests/globals.test.tscode/core/src/preview-api/modules/store/csf/portable-stories.test.tscode/core/src/preview-api/modules/store/csf/normalizeProjectAnnotations.tscode/core/src/csf/story.tscode/core/src/types/modules/csf.tscode/core/src/preview-api/modules/store/GlobalsStore.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/renderers/svelte/src/__test__/composeStories/portable-stories.test.tscode/core/src/manager-api/tests/globals.test.tscode/core/src/preview-api/modules/store/csf/portable-stories.test.tscode/core/src/preview-api/modules/store/csf/normalizeProjectAnnotations.tscode/core/src/csf/story.tscode/core/src/types/modules/csf.tscode/core/src/preview-api/modules/store/GlobalsStore.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/renderers/svelte/src/__test__/composeStories/portable-stories.test.tscode/core/src/manager-api/tests/globals.test.tscode/core/src/preview-api/modules/store/csf/portable-stories.test.tscode/core/src/preview-api/modules/store/csf/normalizeProjectAnnotations.tscode/core/src/csf/story.tscode/core/src/types/modules/csf.tscode/core/src/preview-api/modules/store/GlobalsStore.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{test,spec}.{ts,tsx,js,jsx}: Write meaningful unit tests that actually import and call the functions being tested, not just verify syntax patterns
Achieve high test coverage of business logic, aiming for 75%+ coverage of statements/lines
Cover all branches, conditions, edge cases, error paths, and different input variations in unit tests
Usevi.mock()to mock file system, loggers, and other external dependencies in tests
Files:
code/renderers/svelte/src/__test__/composeStories/portable-stories.test.tscode/core/src/manager-api/tests/globals.test.tscode/core/src/preview-api/modules/store/csf/portable-stories.test.tscode/core/src/preview-api/modules/store/GlobalsStore.test.ts
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/core/src/manager-api/tests/globals.test.tscode/core/src/preview-api/modules/store/csf/portable-stories.test.tscode/core/src/preview-api/modules/store/csf/normalizeProjectAnnotations.tscode/core/src/csf/story.tscode/core/src/types/modules/csf.tscode/core/src/preview-api/modules/store/GlobalsStore.test.ts
🧠 Learnings (24)
📓 Common learnings
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/manager/components/preview/Toolbar.tsx:102-105
Timestamp: 2025-10-03T07:55:42.639Z
Learning: In code/core/src/manager/components/preview/Toolbar.tsx, we intentionally do not add aria-label/aria-labelledby to StyledToolbar (AbstractToolbar) because the enclosing section is already labeled via an sr-only heading and the toolbar is the only content. Revisit only if real user testing indicates a need.
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.
Applied to files:
code/renderers/svelte/src/__test__/composeStories/portable-stories.test.tscode/core/src/preview-api/modules/store/csf/portable-stories.test.ts
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/renderers/svelte/src/__test__/composeStories/portable-stories.test.ts
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: In viewport tool code, when using the `useGlobals` hook from storybook/manager-api, the third returned value `storyGlobals` is guaranteed by TypeScript to be defined (not undefined/null), making the `in` operator safe to use without additional null checks.
Applied to files:
code/renderers/svelte/src/__test__/composeStories/portable-stories.test.tscode/core/src/manager-api/tests/globals.test.tscode/core/src/preview-api/modules/store/csf/normalizeProjectAnnotations.tscode/core/src/preview-api/modules/store/GlobalsStore.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors in Vitest tests
Applied to files:
code/renderers/svelte/src/__test__/composeStories/portable-stories.test.tscode/core/src/preview-api/modules/store/GlobalsStore.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in `beforeEach` blocks in Vitest tests
Applied to files:
code/renderers/svelte/src/__test__/composeStories/portable-stories.test.tscode/core/src/preview-api/modules/store/GlobalsStore.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mock implementations outside of `beforeEach` blocks in Vitest tests
Applied to files:
code/renderers/svelte/src/__test__/composeStories/portable-stories.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses in Vitest tests
Applied to files:
code/renderers/svelte/src/__test__/composeStories/portable-stories.test.tscode/core/src/preview-api/modules/store/GlobalsStore.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases in Vitest tests
Applied to files:
code/renderers/svelte/src/__test__/composeStories/portable-stories.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies in Vitest tests
Applied to files:
code/renderers/svelte/src/__test__/composeStories/portable-stories.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Group related mocks together in Vitest tests
Applied to files:
code/renderers/svelte/src/__test__/composeStories/portable-stories.test.tscode/core/src/preview-api/modules/store/GlobalsStore.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Keep mock implementations simple and focused in Vitest tests
Applied to files:
code/renderers/svelte/src/__test__/composeStories/portable-stories.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock at the highest level of abstraction needed in Vitest tests
Applied to files:
code/renderers/svelte/src/__test__/composeStories/portable-stories.test.tscode/core/src/preview-api/modules/store/GlobalsStore.test.ts
📚 Learning: 2025-12-22T22:03:40.109Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.109Z
Learning: Applies to **/*.{test,spec}.{ts,tsx,js,jsx} : Write meaningful unit tests that actually import and call the functions being tested, not just verify syntax patterns
Applied to files:
code/renderers/svelte/src/__test__/composeStories/portable-stories.test.ts
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: The useGlobals hook from storybook/manager-api returns a tuple where the third element (storyGlobals) is typed as Globals, not Globals | undefined. This means TypeScript guarantees it's always defined, making the `in` operator safe to use without additional null checks.
Applied to files:
code/core/src/manager-api/tests/globals.test.tscode/core/src/preview-api/modules/store/csf/normalizeProjectAnnotations.tscode/core/src/preview-api/modules/store/GlobalsStore.test.ts
📚 Learning: 2025-10-03T07:55:42.639Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/manager/components/preview/Toolbar.tsx:102-105
Timestamp: 2025-10-03T07:55:42.639Z
Learning: In code/core/src/manager/components/preview/Toolbar.tsx, we intentionally do not add aria-label/aria-labelledby to StyledToolbar (AbstractToolbar) because the enclosing section is already labeled via an sr-only heading and the toolbar is the only content. Revisit only if real user testing indicates a need.
Applied to files:
code/core/src/preview-api/modules/store/csf/portable-stories.test.tscode/core/src/csf/story.tscode/core/src/types/modules/csf.ts
📚 Learning: 2025-12-22T22:03:40.109Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.109Z
Learning: Applies to **/*.{ts,tsx} : Enable TypeScript strict mode across all packages
Applied to files:
code/core/src/preview-api/modules/store/csf/normalizeProjectAnnotations.ts
📚 Learning: 2025-12-22T22:03:40.109Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.109Z
Learning: Applies to code/{renderers}/**/*.{ts,tsx,js,jsx} : Use `logger` from `storybook/internal/client-logger` for client-side logging in browser code
Applied to files:
code/core/src/preview-api/modules/store/csf/normalizeProjectAnnotations.ts
📚 Learning: 2025-09-24T09:39:39.233Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 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/core/src/preview-api/modules/store/GlobalsStore.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `vi.mocked()` to type and access the mocked functions in Vitest tests
Applied to files:
code/core/src/preview-api/modules/store/GlobalsStore.test.ts
📚 Learning: 2025-12-22T22:03:40.109Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.109Z
Learning: Applies to **/*.{test,spec}.{ts,tsx,js,jsx} : Use `vi.mock()` to mock file system, loggers, and other external dependencies in tests
Applied to files:
code/core/src/preview-api/modules/store/GlobalsStore.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use type-safe mocking with `vi.mocked()` in Vitest tests
Applied to files:
code/core/src/preview-api/modules/store/GlobalsStore.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `vi.mock()` with the `spy: true` option for all package and file mocks in Vitest tests
Applied to files:
code/core/src/preview-api/modules/store/GlobalsStore.test.ts
📚 Learning: 2025-11-24T17:49:59.279Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.279Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid direct function mocking without `vi.mocked()` in Vitest tests
Applied to files:
code/core/src/preview-api/modules/store/GlobalsStore.test.ts
🧬 Code graph analysis (4)
code/renderers/svelte/src/__test__/composeStories/portable-stories.test.ts (2)
code/core/src/csf/story.ts (1)
ToolbarArgType(148-148)code/core/src/types/modules/csf.ts (1)
ToolbarArgType(68-68)
code/core/src/manager-api/tests/globals.test.ts (2)
code/core/src/csf/story.ts (1)
ToolbarArgType(148-148)code/core/src/types/modules/csf.ts (1)
ToolbarArgType(68-68)
code/core/src/preview-api/modules/store/csf/portable-stories.test.ts (2)
code/core/src/csf/story.ts (1)
ToolbarArgType(148-148)code/core/src/types/modules/csf.ts (1)
ToolbarArgType(68-68)
code/core/src/preview-api/modules/store/csf/normalizeProjectAnnotations.ts (2)
code/core/src/csf/story.ts (1)
StrictGlobalTypes(173-175)code/core/src/types/modules/csf.ts (1)
StrictGlobalTypes(65-65)
⏰ 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 (6)
code/core/src/types/modules/csf.ts (1)
68-68: LGTM! Type export follows existing pattern.The addition of
ToolbarArgTypeto the public type exports is clean and consistent with the existing export structure.code/core/src/preview-api/modules/store/GlobalsStore.test.ts (2)
37-38: Consider whether type assertions are masking type incompatibilities.Multiple type assertions
as ToolbarArgTypeare used throughout this test file. While acceptable in tests, these assertions bypass TypeScript's type checking and could hide actual type mismatches.Verify that:
- The objects being cast actually conform to
ToolbarArgTypestructureToolbarArgTypehas all properties optional (allowing minimal objects like{})- These casts are intentional for testing purposes rather than working around type errors
If
ToolbarArgTypeis intended to accept minimal objects, consider adding a comment explaining why type assertions are necessary in tests.Also applies to: 54-54, 67-67, 85-85, 111-112, 133-133, 151-151, 169-169, 193-193
3-3: The import path'../../../toolbar'is actually consistent with the dominant pattern in the codebase. Most files (5 out of 7) use relative paths to thetoolbardirectory. Only one file in the Svelte renderer uses'storybook/internal/csf', making it the outlier. Both paths are valid sinceToolbarArgTypeis re-exported through the CSF module, but the current relative import follows the established convention.Likely an incorrect or invalid review comment.
code/renderers/svelte/src/__test__/composeStories/portable-stories.test.ts (1)
6-6: Correct public import path used.This file correctly imports
ToolbarArgTypefrom'storybook/internal/csf', which is the appropriate public API for renderer packages.code/core/src/csf/story.ts (2)
3-3: Type import and export look correct.The import of
ToolbarArgTypefrom the toolbar module and its re-export as a public type are implemented correctly.Also applies to: 148-148
173-175: No action needed—the intersection type is well-formed.The
StrictInputType & ToolbarArgTypeintersection has no conflicting property definitions. Both types share the same inherited properties fromInputTypewithout incompatibilities. The deprecateddefaultValueproperty does not create a conflict; it appears identically in both types.
| export interface GlobalTypes { | ||
| [name: string]: InputType; | ||
| [name: string]: ToolbarArgType; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the file and understand the current state
cd /repo
cat -n code/core/src/csf/story.ts | head -180 | tail -30Repository: storybookjs/storybook
Length of output: 1033
🏁 Script executed:
# Find InputType and ToolbarArgType definitions
rg -n "type InputType|type ToolbarArgType|interface InputType|interface ToolbarArgType" --type ts -A 3Repository: storybookjs/storybook
Length of output: 586
🏁 Script executed:
# Find all usages of GlobalTypes
rg -n "GlobalTypes" --type ts -C 2Repository: storybookjs/storybook
Length of output: 10410
🏁 Script executed:
# Get complete InputType and StrictInputType definitions
rg -n "export.*interface InputType|export.*type InputType|export.*interface StrictInputType|export.*type StrictInputType" --type ts -A 20 | head -100Repository: storybookjs/storybook
Length of output: 2856
🏁 Script executed:
# Check ToolbarConfig definition
rg -n "type ToolbarConfig|interface ToolbarConfig" --type ts -A 5Repository: storybookjs/storybook
Length of output: 432
🏁 Script executed:
# Check how normalizeInputTypes handles GlobalTypes
cat -n code/core/src/preview-api/modules/store/csf/normalizeInputTypes.tsRepository: storybookjs/storybook
Length of output: 1433
🏁 Script executed:
# Search for example globalTypes definitions
rg -n "globalTypes.*=" --type ts -A 3 | head -80Repository: storybookjs/storybook
Length of output: 4268
🏁 Script executed:
# Search for any migration notes or documentation about this change
fd -e md | xargs rg -l "globalTypes|GlobalTypes" | head -10Repository: storybookjs/storybook
Length of output: 292
🏁 Script executed:
# Check the example of globalTypes definition
cat -n code/core/template/stories/preview.ts | sed -n '52,70p'Repository: storybookjs/storybook
Length of output: 759
🏁 Script executed:
# Check the documented snippet for globalTypes
cat docs/_snippets/storybook-preview-configure-globaltypes.mdRepository: storybookjs/storybook
Length of output: 2957
🏁 Script executed:
# Check MIGRATION.md for any notes about GlobalTypes changes
rg -n "GlobalTypes|globalTypes" MIGRATION.md -B 2 -A 2Repository: storybookjs/storybook
Length of output: 1329
Backward compatibility broken by required toolbar property.
Changing GlobalTypes from InputType to ToolbarArgType adds a required toolbar property, which is a breaking change. While ToolbarArgType is a superset of InputType and includes all existing properties, users with existing globalTypes definitions that omit the toolbar property will see type errors.
All documented examples and template code include toolbar configurations, and ToolbarManager.tsx defensively filters entries by !!globalTypes[id].toolbar, confirming awareness of this constraint. Users will need to add toolbar configuration to all globalTypes entries or adjust their type annotations.
🤖 Prompt for AI Agents
In code/core/src/csf/story.ts around lines 170 to 172, the index signature was
changed to ToolbarArgType which makes the toolbar property required and breaks
backward compatibility; change the index type to allow both the old InputType
and the new ToolbarArgType (e.g. use InputType | ToolbarArgType or keep
InputType and make toolbar optional on ToolbarArgType) so existing globalTypes
without toolbar do not error, and update any related type exports/tests/comments
accordingly.
| return { | ||
| ...(argTypes && { argTypes: normalizeInputTypes(argTypes as ArgTypes) }), | ||
| ...(globalTypes && { globalTypes: normalizeInputTypes(globalTypes) }), | ||
| ...(globalTypes && { globalTypes: normalizeInputTypes(globalTypes) as StrictGlobalTypes }), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the normalizeInputTypes function definition and check its return type
ast-grep --pattern 'function normalizeInputTypes($$$) {
$$$
}'Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
# Search for normalizeInputTypes usage and context
rg -n "normalizeInputTypes" --type ts -A 3 -B 3Repository: storybookjs/storybook
Length of output: 7572
🏁 Script executed:
# Find the StrictGlobalTypes type definition
rg -n "type StrictGlobalTypes" --type ts -A 5Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
# Check the file structure around line 31
cat -n code/core/src/preview-api/modules/store/csf/normalizeProjectAnnotations.ts | sed -n '1,50p'Repository: storybookjs/storybook
Length of output: 2713
🏁 Script executed:
# Find type definitions for ArgTypes, GlobalTypes, StrictArgTypes, StrictGlobalTypes
rg -n "type (ArgTypes|GlobalTypes|StrictArgTypes|StrictGlobalTypes)" --type tsRepository: storybookjs/storybook
Length of output: 1033
🏁 Script executed:
# Check the normalizeInputTypes implementation more closely
cat -n code/core/src/preview-api/modules/store/csf/normalizeInputTypes.ts | head -50Repository: storybookjs/storybook
Length of output: 1433
🏁 Script executed:
# Look at the normalizeInputType function to understand the logic
cat -n code/core/src/preview-api/modules/store/csf/normalizeInputTypes.tsRepository: storybookjs/storybook
Length of output: 1433
🏁 Script executed:
# Check if there are any type overloads or declarations for normalizeInputTypes
rg -n "declare.*normalizeInputTypes|overload.*normalizeInputTypes" --type tsRepository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
# Find the type definitions for ArgTypes and GlobalTypes
rg -n "export type ArgTypes|export type GlobalTypes|export type StrictArgTypes|export type StrictGlobalTypes" --type ts -A 1Repository: storybookjs/storybook
Length of output: 715
🏁 Script executed:
# Also check if there are interface definitions
rg -n "export interface (ArgTypes|GlobalTypes|StrictArgTypes|StrictGlobalTypes)" --type tsRepository: storybookjs/storybook
Length of output: 468
🏁 Script executed:
# Get the full type definitions
cat -n code/core/src/csf/story.ts | sed -n '164,180p'Repository: storybookjs/storybook
Length of output: 658
🏁 Script executed:
# Check if GlobalTypes and ArgTypes are structurally compatible
cat -n code/core/src/csf/story.ts | sed -n '164,195p'Repository: storybookjs/storybook
Length of output: 1230
🏁 Script executed:
# Look at the issue comment in the code - there seems to be a TODO around line 14-17
git log --oneline code/core/src/preview-api/modules/store/csf/normalizeProjectAnnotations.ts | head -20Repository: storybookjs/storybook
Length of output: 126
🏁 Script executed:
# Find ToolbarArgType definition
rg -n "type ToolbarArgType|interface ToolbarArgType" --type ts -A 3Repository: storybookjs/storybook
Length of output: 273
🏁 Script executed:
# Check InputType and StrictInputType definitions to understand the difference
rg -n "export type InputType|export type StrictInputType" --type ts -A 2Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
# Search for InputType and StrictInputType definitions more broadly
rg -n "export type InputType|export type StrictInputType|export interface InputType|export interface StrictInputType" --type tsRepository: storybookjs/storybook
Length of output: 208
🏁 Script executed:
# Check what ToolbarArgType extends/uses
cat -n code/core/src/toolbar/types.ts | head -60Repository: storybookjs/storybook
Length of output: 1774
🏁 Script executed:
# Check the InputType and StrictInputType definitions
cat -n code/core/src/csf/story.ts | sed -n '111,160p'Repository: storybookjs/storybook
Length of output: 2159
🏁 Script executed:
# Check what types argTypes and globalTypes have in ProjectAnnotations
rg -n "interface ProjectAnnotations|type ProjectAnnotations" --type ts -A 15Repository: storybookjs/storybook
Length of output: 2934
🏁 Script executed:
# Find BaseAnnotations to see the types of argTypes and globalTypes
rg -n "interface BaseAnnotations|type BaseAnnotations" --type ts -A 20Repository: storybookjs/storybook
Length of output: 2452
🏁 Script executed:
# Also check NormalizedProjectAnnotations more fully
cat -n code/core/src/types/modules/story.ts | sed -n '55,65p'Repository: storybookjs/storybook
Length of output: 534
🏁 Script executed:
# Find argTypes and globalTypes properties in BaseAnnotations/ProjectAnnotations
cat -n code/core/src/csf/story.ts | sed -n '331,405p'Repository: storybookjs/storybook
Length of output: 3412
🏁 Script executed:
# Check if globalTypes is defined in ProjectAnnotations
cat -n code/core/src/csf/story.ts | sed -n '405,460p'Repository: storybookjs/storybook
Length of output: 3112
🏁 Script executed:
# Check what properties are actually preserved by normalizeInputType
cat -n code/core/src/preview-api/modules/store/csf/normalizeInputTypes.tsRepository: storybookjs/storybook
Length of output: 1433
🏁 Script executed:
# Check if there are any related issues or PRs mentioning this
git log --all --oneline --grep="GlobalTypes\|normalizeInputTypes" | head -20Repository: storybookjs/storybook
Length of output: 147
🏁 Script executed:
# Check the tests to understand the expected behavior
cat -n code/core/src/preview-api/modules/store/csf/normalizeInputTypes.test.ts | sed -n '68,100p'Repository: storybookjs/storybook
Length of output: 471
Add function overloads to normalizeInputTypes to enable proper type narrowing.
The type assertion as StrictGlobalTypes is necessary because normalizeInputTypes returns a union type StrictArgTypes | StrictGlobalTypes without overloads to narrow it based on the input. The function should declare overloads to automatically provide the correct return type:
normalizeInputTypes(ArgTypes)→StrictArgTypesnormalizeInputTypes(GlobalTypes)→StrictGlobalTypes
This would eliminate the need for the output cast and improve type safety throughout the codebase.
🤖 Prompt for AI Agents
In code/core/src/preview-api/modules/store/csf/normalizeProjectAnnotations.ts
around line 31, the call uses a cast to StrictGlobalTypes because
normalizeInputTypes currently returns a union (StrictArgTypes |
StrictGlobalTypes); add TypeScript overloads to normalizeInputTypes so that
calling it with ArgTypes returns StrictArgTypes and calling it with GlobalTypes
returns StrictGlobalTypes, then remove the cast here; implement two function
signatures (one accepting ArgTypes and returning StrictArgTypes, another
accepting GlobalTypes and returning StrictGlobalTypes) followed by the single
implementation that handles both inputs and returns the properly narrowed type.
2 participants
What I did
GlobalTypesvalues were previously typed asInputType. This meant that indefinePreview, for example, the toolbar options were not correctly type checked.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Not required
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 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 publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
ToolbarArgTypeis now exported for developer use in global argument definitions.✏️ Tip: You can customize this high-level summary in your review settings.