TypeScript: Improve globalTypes type-strictness #33313
Conversation
| export type ToolbarConfig = NormalizedToolbarConfig & { | ||
| items: string[] | ToolbarItem[]; | ||
| export type ToolbarConfig = Omit<NormalizedToolbarConfig, 'items'> & { | ||
| items: (string | ToolbarItem)[]; |
There was a problem hiding this comment.
Loosened this to allow for a mix
mrginglymus
force-pushed
the
globalTypes
branch
4 times, most recently
from
da158df to
1478ce5
Compare
| /** A smart component for handling manager-preview interactions. */ | ||
| export const ToolbarManager: FC = () => { | ||
| const globalTypes = useGlobalTypes(); | ||
| const globalIds = Object.keys(globalTypes).filter((id) => !!globalTypes[id].toolbar); |
There was a problem hiding this comment.
Honestly, lots of other ways of acheiving type safety here, this was just the quickest.
mrginglymus
force-pushed
the
globalTypes
branch
from
1478ce5 to
3276cef
Compare
| cycleValues.current = createCycleValueArray(items); | ||
| }, []); | ||
|
|
||
| return <Component cycleValues={cycleValues.current} {...props} />; |
There was a problem hiding this comment.
It's safe to say that cycleValues is discarded though
mrginglymus
force-pushed
the
globalTypes
branch
from
ca9f9f1 to
644b6b0
Compare
code/core/src/preview-api/modules/store/csf/getValuesFromGlobalTypes.ts
Outdated
Show resolved
Hide resolved
|
View your CI Pipeline Execution ↗ for commit 9ce7be2 ☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughSwitch default global derivation from argTypes to globalTypes; change GlobalTypes to use ToolbarArgType and remove StrictGlobalTypes; stop normalizing globalTypes in project annotations; refactor toolbar types and components to hooks-based implementations with integrated shortcut and reset handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Toolbar as ToolbarMenuSelect
participant API as StorybookAPI
participant Globals as GlobalsStore
Note right of Toolbar: UI interactions & keyboard shortcuts
User->>Toolbar: select / reset item or press shortcut
Toolbar->>API: api.setGlobals({ globals })
API->>Globals: setGlobals(payload)
Globals-->>API: updated state
API-->>Toolbar: hook notifies updated globals
Toolbar-->>User: update selection, icon, title
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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/toolbar/utils/register-shortcuts.ts (1)
6-10: Properties should be marked as optional to match runtime behavior.The
Shortcutsinterface definesnext,previous, andresetas required properties, but the runtime code (lines 13, 22, 31) conditionally checks for their presence before using them. This mismatch creates an overly restrictive type that forces callers to provide all three shortcuts even when only one or two are needed.🔎 Proposed fix to make properties optional
interface Shortcuts { - next: ToolbarShortcutConfig & { action: () => void }; - previous: ToolbarShortcutConfig & { action: () => void }; - reset: ToolbarShortcutConfig & { action: () => void }; + next?: ToolbarShortcutConfig & { action: () => void }; + previous?: ToolbarShortcutConfig & { action: () => void }; + reset?: ToolbarShortcutConfig & { action: () => void }; }
🧹 Nitpick comments (2)
code/core/src/toolbar/utils/normalize-toolbar-arg-type.ts (1)
20-21: Minor inconsistency: use localtoolbarvariable.Line 21 uses
argType.toolbarwhile the rest of the function uses the localtoolbarvariable. For consistency, prefer using the local variable.🔎 Proposed fix
toolbar: { - ...argType.toolbar, + ...toolbar, items: toolbar.items.map((_item) => {code/core/src/toolbar/components/ToolbarMenuSelect.tsx (1)
90-118: Consider narrowing effect dependencies for performance.The effect depends on the full
globalsobject but only accessesglobals[id]. This may cause unnecessary shortcut re-registrations when unrelated globals change. Consider extractingglobals[id]to a variable before the effect.🔎 Proposed optimization
const currentValue = globals[id]; const isOverridden = id in storyGlobals; + const currentGlobal = globals[id]; // ...existing code... React.useEffect(() => { if (shortcuts) { const length = options.length; void registerShortcuts(api, id, { next: { ...shortcuts.next, action: () => { - const idx = options.findIndex((i) => i.value === globals[id]); + const idx = options.findIndex((i) => i.value === currentGlobal); const nextIdx = idx < 0 ? 0 : (idx + 1) % length; updateGlobals({ [id]: options[nextIdx].value }); }, }, previous: { ...shortcuts.previous, action: () => { - const idx = options.findIndex((i) => i.value === globals[id]); + const idx = options.findIndex((i) => i.value === currentGlobal); const previousIdx = idx < 0 ? length - 1 : (idx + length - 1) % length; updateGlobals({ [id]: options[previousIdx].value }); }, }, // ...reset unchanged... }); } - }, [api, id, shortcuts, globals, options, updateGlobals]); + }, [api, id, shortcuts, currentGlobal, options, updateGlobals]);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
code/core/src/csf/story.tscode/core/src/manager-api/modules/globals.tscode/core/src/manager-api/root.tsxcode/core/src/manager-api/tests/globals.test.tscode/core/src/preview-api/modules/store/GlobalsStore.tscode/core/src/preview-api/modules/store/StoryStore.test.tscode/core/src/preview-api/modules/store/csf/getValuesFromArgTypes.tscode/core/src/preview-api/modules/store/csf/getValuesFromGlobalTypes.tscode/core/src/preview-api/modules/store/csf/index.tscode/core/src/preview-api/modules/store/csf/normalizeInputTypes.tscode/core/src/preview-api/modules/store/csf/normalizeProjectAnnotations.tscode/core/src/preview-api/modules/store/csf/portable-stories.tscode/core/src/toolbar/components/ToolbarManager.tsxcode/core/src/toolbar/components/ToolbarMenuSelect.tsxcode/core/src/toolbar/hoc/withKeyboardCycle.tsxcode/core/src/toolbar/types.tscode/core/src/toolbar/utils/create-cycle-value-array.tscode/core/src/toolbar/utils/get-selected.tscode/core/src/toolbar/utils/normalize-toolbar-arg-type.tscode/core/src/toolbar/utils/register-shortcuts.tscode/core/src/types/modules/csf.tscode/core/src/types/modules/story.ts
💤 Files with no reviewable changes (6)
- code/core/src/types/modules/csf.ts
- code/core/src/types/modules/story.ts
- code/core/src/preview-api/modules/store/csf/getValuesFromArgTypes.ts
- code/core/src/toolbar/utils/create-cycle-value-array.ts
- code/core/src/preview-api/modules/store/csf/normalizeProjectAnnotations.ts
- code/core/src/toolbar/hoc/withKeyboardCycle.tsx
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{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/core/src/toolbar/utils/register-shortcuts.tscode/core/src/preview-api/modules/store/csf/getValuesFromGlobalTypes.tscode/core/src/preview-api/modules/store/GlobalsStore.tscode/core/src/toolbar/components/ToolbarManager.tsxcode/core/src/preview-api/modules/store/StoryStore.test.tscode/core/src/toolbar/utils/normalize-toolbar-arg-type.tscode/core/src/csf/story.tscode/core/src/toolbar/components/ToolbarMenuSelect.tsxcode/core/src/preview-api/modules/store/csf/portable-stories.tscode/core/src/manager-api/modules/globals.tscode/core/src/manager-api/root.tsxcode/core/src/toolbar/utils/get-selected.tscode/core/src/manager-api/tests/globals.test.tscode/core/src/preview-api/modules/store/csf/index.tscode/core/src/preview-api/modules/store/csf/normalizeInputTypes.tscode/core/src/toolbar/types.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/core/src/toolbar/utils/register-shortcuts.tscode/core/src/preview-api/modules/store/csf/getValuesFromGlobalTypes.tscode/core/src/preview-api/modules/store/GlobalsStore.tscode/core/src/toolbar/components/ToolbarManager.tsxcode/core/src/preview-api/modules/store/StoryStore.test.tscode/core/src/toolbar/utils/normalize-toolbar-arg-type.tscode/core/src/csf/story.tscode/core/src/toolbar/components/ToolbarMenuSelect.tsxcode/core/src/preview-api/modules/store/csf/portable-stories.tscode/core/src/manager-api/modules/globals.tscode/core/src/manager-api/root.tsxcode/core/src/toolbar/utils/get-selected.tscode/core/src/manager-api/tests/globals.test.tscode/core/src/preview-api/modules/store/csf/index.tscode/core/src/preview-api/modules/store/csf/normalizeInputTypes.tscode/core/src/toolbar/types.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/toolbar/utils/register-shortcuts.tscode/core/src/preview-api/modules/store/csf/getValuesFromGlobalTypes.tscode/core/src/preview-api/modules/store/GlobalsStore.tscode/core/src/toolbar/components/ToolbarManager.tsxcode/core/src/preview-api/modules/store/StoryStore.test.tscode/core/src/toolbar/utils/normalize-toolbar-arg-type.tscode/core/src/csf/story.tscode/core/src/toolbar/components/ToolbarMenuSelect.tsxcode/core/src/preview-api/modules/store/csf/portable-stories.tscode/core/src/manager-api/modules/globals.tscode/core/src/manager-api/root.tsxcode/core/src/toolbar/utils/get-selected.tscode/core/src/manager-api/tests/globals.test.tscode/core/src/preview-api/modules/store/csf/index.tscode/core/src/preview-api/modules/store/csf/normalizeInputTypes.tscode/core/src/toolbar/types.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/core/src/toolbar/utils/register-shortcuts.tscode/core/src/preview-api/modules/store/csf/getValuesFromGlobalTypes.tscode/core/src/preview-api/modules/store/GlobalsStore.tscode/core/src/toolbar/components/ToolbarManager.tsxcode/core/src/preview-api/modules/store/StoryStore.test.tscode/core/src/toolbar/utils/normalize-toolbar-arg-type.tscode/core/src/csf/story.tscode/core/src/toolbar/components/ToolbarMenuSelect.tsxcode/core/src/preview-api/modules/store/csf/portable-stories.tscode/core/src/manager-api/modules/globals.tscode/core/src/manager-api/root.tsxcode/core/src/toolbar/utils/get-selected.tscode/core/src/manager-api/tests/globals.test.tscode/core/src/preview-api/modules/store/csf/index.tscode/core/src/preview-api/modules/store/csf/normalizeInputTypes.tscode/core/src/toolbar/types.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/toolbar/utils/register-shortcuts.tscode/core/src/preview-api/modules/store/csf/getValuesFromGlobalTypes.tscode/core/src/preview-api/modules/store/GlobalsStore.tscode/core/src/toolbar/components/ToolbarManager.tsxcode/core/src/preview-api/modules/store/StoryStore.test.tscode/core/src/toolbar/utils/normalize-toolbar-arg-type.tscode/core/src/csf/story.tscode/core/src/toolbar/components/ToolbarMenuSelect.tsxcode/core/src/preview-api/modules/store/csf/portable-stories.tscode/core/src/manager-api/modules/globals.tscode/core/src/manager-api/root.tsxcode/core/src/toolbar/utils/get-selected.tscode/core/src/manager-api/tests/globals.test.tscode/core/src/preview-api/modules/store/csf/index.tscode/core/src/preview-api/modules/store/csf/normalizeInputTypes.tscode/core/src/toolbar/types.ts
**/*.{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/core/src/preview-api/modules/store/StoryStore.test.tscode/core/src/manager-api/tests/globals.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/core/src/preview-api/modules/store/StoryStore.test.tscode/core/src/manager-api/tests/globals.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/core/src/preview-api/modules/store/StoryStore.test.tscode/core/src/manager-api/tests/globals.test.ts
🧠 Learnings (9)
📓 Common learnings
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.
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.
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.
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.
📚 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/toolbar/utils/register-shortcuts.tscode/core/src/toolbar/components/ToolbarManager.tsxcode/core/src/toolbar/components/ToolbarMenuSelect.tsxcode/core/src/toolbar/types.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/preview-api/modules/store/csf/getValuesFromGlobalTypes.tscode/core/src/preview-api/modules/store/GlobalsStore.tscode/core/src/toolbar/components/ToolbarManager.tsxcode/core/src/preview-api/modules/store/StoryStore.test.tscode/core/src/csf/story.tscode/core/src/toolbar/components/ToolbarMenuSelect.tsxcode/core/src/preview-api/modules/store/csf/portable-stories.tscode/core/src/manager-api/modules/globals.tscode/core/src/manager-api/root.tsxcode/core/src/manager-api/tests/globals.test.tscode/core/src/preview-api/modules/store/csf/normalizeInputTypes.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/core/src/preview-api/modules/store/csf/getValuesFromGlobalTypes.tscode/core/src/preview-api/modules/store/GlobalsStore.tscode/core/src/toolbar/components/ToolbarManager.tsxcode/core/src/preview-api/modules/store/StoryStore.test.tscode/core/src/toolbar/components/ToolbarMenuSelect.tsxcode/core/src/preview-api/modules/store/csf/portable-stories.tscode/core/src/manager-api/modules/globals.tscode/core/src/manager-api/root.tsxcode/core/src/manager-api/tests/globals.test.tscode/core/src/preview-api/modules/store/csf/normalizeInputTypes.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/toolbar/components/ToolbarManager.tsxcode/core/src/preview-api/modules/store/StoryStore.test.tscode/core/src/toolbar/components/ToolbarMenuSelect.tsxcode/core/src/preview-api/modules/store/csf/portable-stories.ts
📚 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/core/src/toolbar/components/ToolbarMenuSelect.tsx
📚 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/core/src/toolbar/components/ToolbarMenuSelect.tsx
📚 Learning: 2025-11-05T09:36:55.944Z
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.
Applied to files:
code/core/src/toolbar/components/ToolbarMenuSelect.tsx
📚 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,js,jsx} : Export functions from modules if they need to be tested
Applied to files:
code/core/src/preview-api/modules/store/csf/index.ts
🧬 Code graph analysis (8)
code/core/src/preview-api/modules/store/csf/getValuesFromGlobalTypes.ts (2)
code/core/src/csf/story.ts (2)
GlobalTypes(168-170)Globals(165-167)code/core/src/types/modules/csf.ts (2)
GlobalTypes(26-26)Globals(25-25)
code/core/src/preview-api/modules/store/GlobalsStore.ts (1)
code/core/src/preview-api/modules/store/csf/getValuesFromGlobalTypes.ts (1)
getValuesFromGlobalTypes(3-9)
code/core/src/toolbar/components/ToolbarManager.tsx (2)
code/core/src/toolbar/utils/normalize-toolbar-arg-type.ts (1)
normalizeArgType(8-37)code/core/src/toolbar/components/ToolbarMenuSelect.tsx (1)
ToolbarMenuSelect(28-139)
code/core/src/toolbar/utils/normalize-toolbar-arg-type.ts (1)
code/core/src/toolbar/types.ts (1)
NormalizedToolbarArgType(38-43)
code/core/src/csf/story.ts (1)
code/core/src/toolbar/types.ts (1)
ToolbarArgType(49-54)
code/core/src/preview-api/modules/store/csf/portable-stories.ts (1)
code/core/src/preview-api/modules/store/csf/getValuesFromGlobalTypes.ts (1)
getValuesFromGlobalTypes(3-9)
code/core/src/manager-api/root.tsx (2)
code/core/src/csf/story.ts (1)
GlobalTypes(168-170)code/core/src/types/modules/csf.ts (1)
GlobalTypes(26-26)
code/core/src/preview-api/modules/store/csf/normalizeInputTypes.ts (2)
code/core/src/csf/story.ts (2)
ArgTypes(162-162)StrictArgTypes(163-163)code/core/src/types/modules/csf.ts (2)
ArgTypes(12-12)StrictArgTypes(64-64)
🪛 Biome (2.1.2)
code/core/src/toolbar/components/ToolbarMenuSelect.tsx
[error] 64-64: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
⏰ 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). (1)
- GitHub Check: normal
🔇 Additional comments (19)
code/core/src/toolbar/utils/register-shortcuts.ts (1)
13-13: Simplified null checks are appropriate.Removing the
shortcuts &&checks is valid since theshortcutsparameter is typed as non-nullable. The individual property checks remain necessary if the properties are made optional (as suggested above).Also applies to: 22-22, 31-31
code/core/src/preview-api/modules/store/StoryStore.test.ts (1)
49-49: LGTM! Test expectations correctly reflect simplified globalTypes.The test expectations have been appropriately updated to reflect that globalTypes are no longer normalized with nested type information, now containing only
{ name: 'a' }instead of the previous{ name: 'a', type: { name: 'string' } }. This aligns with the PR's objective to differentiate globalTypes from argTypes normalization.Also applies to: 91-91, 103-103
code/core/src/preview-api/modules/store/csf/getValuesFromGlobalTypes.ts (1)
3-9: LGTM! Function correctly extracts default values from GlobalTypes.The implementation correctly iterates over GlobalTypes entries and collects defined defaultValue properties into a Globals object. The
typeof defaultValue !== 'undefined'check ensures only explicitly set defaults are included.code/core/src/preview-api/modules/store/csf/portable-stories.ts (1)
35-35: LGTM! Correctly updated to use getValuesFromGlobalTypes.The import and usage have been appropriately updated from
getValuesFromArgTypestogetValuesFromGlobalTypes, aligning with the PR's objective to distinguish GlobalTypes handling from ArgTypes.Also applies to: 134-134
code/core/src/preview-api/modules/store/GlobalsStore.ts (1)
5-5: LGTM! Default globals now derived from GlobalTypes.The change from
getValuesFromArgTypestogetValuesFromGlobalTypescorrectly aligns the default globals derivation with the new GlobalTypes-based typing system introduced in this PR.Also applies to: 30-30
code/core/src/manager-api/root.tsx (1)
40-40: LGTM! Public API correctly updated to return GlobalTypes.The
useGlobalTypeshook now returnsGlobalTypesinstead ofArgTypes, which is the correct type for global toolbar configurations as per this PR's objectives.Also applies to: 490-492
code/core/src/preview-api/modules/store/csf/index.ts (1)
8-8: LGTM! Public export updated to reflect new utility.The re-export has been correctly updated from
getValuesFromArgTypestogetValuesFromGlobalTypes. This is a public API change, and any external consumers directly importing this utility will need to update their imports.code/core/src/manager-api/modules/globals.ts (1)
56-67: Non-null assertions are justified by state initialization.The code is safe: all four properties (
globals,userGlobals,storyGlobals,globalTypes) are initialized to empty objects in the state object (lines 79-84) before any getter can be called. The optional markers in theSubStateinterface reflect the type system (properties can be undefined in theory), but the actual initialization guarantees these properties are never undefined at runtime. The event handlers (SET_GLOBALSat line 138 andGLOBALS_UPDATEDviaupdateGlobals) further ensure these properties are set when globals are provided. The non-null assertions are appropriate.code/core/src/toolbar/types.ts (2)
29-29: No changes needed. The optionaliconfield is properly handled throughout the toolbar rendering logic. All accesses include appropriate guards:
- Direct accesses use conditional checks (
if (toolbar.icon)) before usage- Property access uses optional chaining with fallbacks (
?.icon || fallback)- Icon rendering uses conditional rendering (
item.icon ?) before component instantiationNo breaking changes detected.
49-54: Update documentation to clarify that ToolbarArgType is an input type with all optional fields, normalized to NormalizedToolbarArgType with required fields.
ToolbarArgTypehas all fields optional by design as an input type. ThenormalizeArgType()function properly handles optional fields by providing fallbacks (e.g.,argType.name || keyat line 18 of normalize-toolbar-arg-type.ts), ensuring the normalized outputNormalizedToolbarArgTypealways has requirednameanddescriptionfields. Consumers safely access normalized types, not the raw input type, making this a sound input/output type design pattern rather than a breaking change.code/core/src/manager-api/tests/globals.test.ts (3)
64-71: LGTM!The test expectation correctly reflects the new
SetGlobalsPayloadshape whereglobalTypesentries are now simple objects conforming toToolbarArgType(with all optional properties) rather than the previousArgTypewith nestedtypeinformation.
89-98: Test expectations aligned with new globalTypes shape.The updated expectations match the simplified
globalTypesstructure across the emit and expected state assertions.
139-142: Consistent with other test updates.This test case for ignoring
SET_GLOBALSfrom other refs correctly uses the newglobalTypesshape.code/core/src/toolbar/components/ToolbarManager.tsx (1)
11-26: LGTM!The refactored logic is clean:
hasToolbarsefficiently checks if any global type defines a toolbar- The render loop correctly guards against null returns from
normalizeArgType- The conditional rendering pattern
normalizedArgType && <ToolbarMenuSelect ... />properly handles entries without toolbarscode/core/src/preview-api/modules/store/csf/normalizeInputTypes.ts (1)
35-36: API surface narrowed correctly.The function now only handles
ArgTypes→StrictArgTypes, removing support forGlobalTypes. This aligns with the PR's goal to stop normalizing globalTypes and treat them differently from argTypes.code/core/src/toolbar/utils/get-selected.ts (1)
8-9: LGTM!The simplified implementation directly returns the find result, which is cleaner. The caller in
ToolbarMenuSelectcorrectly uses optional chaining to handle theundefinedcase.code/core/src/toolbar/components/ToolbarMenuSelect.tsx (2)
28-52: Clean refactor from HOC to hooks pattern.The component is well-structured with proper hook usage. The dynamic icon/title logic is clear and the console.warn for missing icon/title is appropriate for manager UI code.
124-138: LGTM!The Select component integration is clean with proper prop handling. The static analysis hint about a missing key on line 64 is a false positive—that line is inside a
.map()callback building option objects, not JSX elements in a list. The actual rendering is handled by the Select component internally.code/core/src/csf/story.ts (1)
168-170: Breaking type change: GlobalTypes now uses ToolbarArgType instead of InputType.
GlobalTypesnow maps toToolbarArgType(withname?,description?,defaultValue?,toolbar?) instead ofInputType. This removes properties includingcontrol,type,mapping,options,table, and the permissive index signature[key: string]: any.Code using the removed properties will now have type errors, which is the intended behavior per the PR objectives. Downstream consumers have been updated to work with the narrower type—for example,
getValuesFromGlobalTypesonly accessesdefaultValue, which is present inToolbarArgType.
|
@mrginglymus Can we have a quick meeting about this PR some time? I find it a bit hard to wrap my head around, and think it might be useful to talk it over in a short 1:1 meeting or perhaps during a upcoming triage meeting? |
…lTypes.ts Co-authored-by: Norbert de Langen <ndelangen@me.com>
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/toolbar/types.ts (1)
1-1: Remove unusedInputTypeimport.The import on line 1 is no longer used in any type definitions since
NormalizedToolbarArgTypeno longer extendsInputType. AlthoughInputTypeis mentioned in the deprecation comment on line 55, it is not referenced in actual code and can be safely removed.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/toolbar/types.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/core/src/toolbar/types.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/core/src/toolbar/types.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/toolbar/types.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/core/src/toolbar/types.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/toolbar/types.ts
🧠 Learnings (1)
📚 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/toolbar/types.ts
🧬 Code graph analysis (1)
code/core/src/toolbar/types.ts (1)
code/core/src/components/components/icon/icon.tsx (1)
IconsProps(25-30)
⏰ 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/core/src/toolbar/types.ts (4)
29-29: Makingiconoptional is appropriate.This aligns with the broader PR changes where toolbar icons may not always be present. The change is consistent with the optional nature of icons in
ToolbarItem(line 18).
45-47: Good distinction between user-facing and normalized item types.
ToolbarConfig.itemsaccepting(string | ToolbarItem)[]for user input whileNormalizedToolbarConfig.itemsrequiresToolbarItem[]is the correct pattern—strings get normalized to full objects.
49-59: Clear deprecation notice and backwards compatibility approach.The deprecated index signature with a clear removal timeline (Storybook 11) provides a good migration path for users who relied on arbitrary extra properties. This aligns with the PR's intent to provide stricter typing while maintaining compatibility.
38-43: Normalization correctly provides defaults for required fields.The
normalizeArgTypefunction incode/core/src/toolbar/utils/normalize-toolbar-arg-type.tsproperly handles the conversion from optional to required fields:
namedefaults to thekeyparameter when undefined (line 18)descriptiondefaults to thekeyparameter when undefined (line 19)The usage pattern in
ToolbarManager.tsxensures the normalized type is only consumed after the normalization step, preventing any exposure of incomplete data.
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 49 | 49 | 0 |
| Self size | 20.30 MB | 20.32 MB | 🚨 +22 KB 🚨 |
| Dependency size | 16.52 MB | 16.52 MB | 🎉 -4 B 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 538 | 538 | 0 |
| Self size | 646 KB | 646 KB | 🎉 -10 B 🎉 |
| Dependency size | 59.22 MB | 59.28 MB | 🚨 +58 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 127 | 127 | 0 |
| Self size | 1.12 MB | 1.12 MB | 🎉 -22 B 🎉 |
| Dependency size | 21.82 MB | 21.87 MB | 🚨 +49 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 159 | 159 | 0 |
| Self size | 30 KB | 30 KB | 🚨 +8 B 🚨 |
| Dependency size | 23.00 MB | 23.17 MB | 🚨 +164 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 117 | 117 | 0 |
| Self size | 35 KB | 35 KB | 🎉 -8 B 🎉 |
| Dependency size | 19.62 MB | 19.66 MB | 🚨 +49 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 278 | 278 | 0 |
| Self size | 24 KB | 24 KB | 🚨 +2 B 🚨 |
| Dependency size | 44.13 MB | 44.19 MB | 🚨 +58 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 183 | 183 | 0 |
| Self size | 775 KB | 775 KB | 🚨 +139 B 🚨 |
| Dependency size | 67.38 MB | 67.47 MB | 🚨 +91 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 176 | 176 | 0 |
| Self size | 30 KB | 30 KB | 🎉 -40 B 🎉 |
| Dependency size | 65.95 MB | 66.04 MB | 🚨 +91 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 50 | 50 | 0 |
| Self size | 1000 KB | 1000 KB | 🎉 -6 B 🎉 |
| Dependency size | 36.82 MB | 36.84 MB | 🚨 +22 KB 🚨 |
| Bundle Size Analyzer | node | node |
@storybook/react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 57 | 57 | 0 |
| Self size | 732 KB | 781 KB | 🚨 +49 KB 🚨 |
| Dependency size | 12.94 MB | 12.94 MB | 🚨 +403 B 🚨 |
| Bundle Size Analyzer | Link | Link |
3 participants
What I did
globalTypeswas previously incorrectly typed asRecord<string, ArgType>.They share in common only
nameanddescription. Both also havedefaultValue, though this is deprecated inArgTypebut essential inGlobalType. Everything else inArgTypeis irrelevant toGlobalType, and is missing typing for thetoolbarprop.This PR attempts to untangle this by retyping
globalTypesasRecord<string, GlobalType>. There were a number of unsafe casts that were removed as part of this, which also led to the discovery of a seemingly unused HOC.Behavioural Changes:
global types are no longer normalised. The only change here is that
namemay now be undefined. However, name is only referenced twice:https://github.com/mrginglymus/storybook/blob/23326cfd31a16ae4f748c0c44cf33cd17e15b838/code/core/src/toolbar/components/ToolbarMenuSelect.tsx#L48-L50
https://github.com/mrginglymus/storybook/blob/23326cfd31a16ae4f748c0c44cf33cd17e15b838/code/core/src/toolbar/components/ToolbarMenuSelect.tsx#L86
so this is unlikely to be an issue
Type Changes
There are potentially breaking type changes. These are:
removal of
[key: string]: anytype on global types. This means users passing in unexpected extra properties will receive an error. However, these extra properties were completely unused anyway.This could be fixed by adding the
anyback in, but that will just lead to more type-rotacurate typing of
toolbar. Anyone for whom this is an issue is likely to have experienced run-time errors too...Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
To test keyboard shortcuts:
yarn storybook:uiLandKcycle up and down languages as expectedTo test typing:
globalTypesdo not raise type errors, but are marked as deprecated-->
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
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.