Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. WalkthroughThe changes broaden CSF addon parameter and tag typing. Numeric addon parameter values are now accepted. ChangesCSF addon typing
Merge Risk: ⚪ Minimal · up to This change broadens addon tag and parameter typing to support renderer-specific tags and numeric values, with type-test coverage for strict and extensible tag unions. No current merge-blocking risk is identified. Comment |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Can we land this in 11? |
There was a problem hiding this comment.
🤖 I am the Storybook contribution review bot. This message was posted automatically. A human maintainer makes the final decision on this PR.
Review phase: Architecture review (stage 1 of 3) and Thermo-nuclear code quality review (stage 2 of 3)We are trying a three-stage automated review: architecture, code quality, and runtime verification. This message states which stages ran and which were skipped. Messages are posted automatically, and a human maintainer makes the final decision. Please tell us whether the feedback and evidence are useful.
Thanks @unional for tracking this down! Changing AddonTypes.tags to unknown is the right fix in the right place. unknown drops out of the intersection, so a custom tag type from .type<…>() now passes through unchanged and autocompletion works. We compared TypeScript completions with the base and head story.ts to confirm this.
Result: changes requested. Both blocking items are in the tests: the new react-vite test file fails that package's type check, and the edits to csf-factories.test.ts remove existing coverage. The inline comments have the details, plus a few optional suggestions.
- Ran on
a1364691: architecture review and code quality review, including type checks for core, react, react-vite, web-components, html, preact, nextjs-vite, angular, tanstack-react, vue3 (vue-tsc) and svelte (svelte-check). - Not run yet: runtime verification (stage 3), which runs after the blocking items are fixed. Also not run: oxlint, type checks for the other frameworks (for example vue3-vite, sveltekit, angular-vite, nextjs, react-webpack5), runtime tests, and the CI status check.
`AddonTypes.tags` is now `unknown`, so a custom tags type from an addon or `.type<...>()` passes through unchanged instead of intersecting with `Tag[]`, which broke autocompletion. Move the coverage into the core `customize tags type` suite: exact-type assertions that fail without the fix, and a strict-union negative case. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
🤖 I am the Storybook contribution review bot. This message was posted automatically. A human maintainer makes the final decision on this PR.
Review phase: Final verdict: Architecture review (stage 1 of 3), Thermo-nuclear code quality review (stage 2 of 3) and Runtime verification (stage 3 of 3)We are trying a three-stage automated review: architecture, code quality, and runtime verification. This message states which stages ran and which were skipped. Messages are posted automatically, and a human maintainer makes the final decision. Please tell us whether the feedback and evidence are useful.
Thanks @unional for the quick and careful follow-up! Moving the tests into the core customize tags type suite and bringing back the react-vite test with exact-type checks closes every item from the first round.
Result: verified. All three stages pass on e0854801. Nothing is blocking. There is one optional suggestion inline.
- Bug reproduced on the base branch, fixed on this head. With the base
story.ts, TypeScript offers no completions attags: ['|']for tags declared through.type<…>()ordefinePreviewAddon<…>. With this PR it offers'foo'and'bar'. We checked this in the repo and in a small project built against the compiled packages. - The tests guard the fix. With the base
story.ts, the type check fails atcsf-factories.test.tsL244 and L274 and atdefine-preview.test.tsL38. On this head, the type checks for core and react-vite pass, and the two test files pass in Vitest. - No regressions found. Without custom tags, any string tag is still accepted. Strict tag unions still reject unknown tags. Type checks also pass for react, web-components, html, preact, nextjs-vite, addon-a11y, addon-docs and vue3 (
vue-tsc). - Not run: a full sandbox or browser run, editor UI checks (completions were checked with the TypeScript language service), oxlint, and type checks for frameworks not listed above.
Cover the fix through the React renderer's own `meta.story` overloads. The exact-type assertion fails without the `AddonTypes.tags` change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
🤖 I am the Storybook contribution review bot. This message was posted automatically; a human maintainer makes the final decision.
Review phase: Architecture review (stage 1 of 3) and Thermo-nuclear code quality review (stage 2 of 3)We are trying a three-stage automated review: architecture, code quality, and runtime verification. This message states which stages ran and which were skipped. Messages are posted automatically, and a human maintainer makes the final decision. Please tell us whether the feedback and evidence are useful.
Thanks @unional! We re-reviewed the latest push (48ab7eeb). It changes one line in define-preview.test.ts.
Result: one blocking item. Moving the @ts-expect-error above meta.story({ makes the react-vite type check fail again. The details and the one-line fix are in the inline comment. Everything else in the PR is unchanged since the verified head e0854801, so the earlier passes for story.ts and csf-factories.test.ts still apply.
I also replied in the existing thread about the strict-set test. You were right that the file as a whole guards the fix, and that suggestion stays optional.
- Stages run: architecture and code quality, scoped to this push.
- Not run: runtime verification (stage 3). The verification on
e0854801doesn't cover this head, so it runs again after the type check passes.
| tags: ['foo', 'bar'], | ||
| }); | ||
|
|
||
| // @ts-expect-error - bad tags |
There was a problem hiding this comment.
Blocking: @ts-expect-error only covers the next line, which is meta.story({ (L21). TypeScript reports the bad tags one line lower, on tags: ['baz', 'qux'], (L22).
Effect: the directive is unused and the real errors are left uncovered, so the react-vite type check (yarn exec jiti ./scripts/check/check-package.ts --cwd code/frameworks/react-vite) exits 1: TS2578 (unused directive) at 20:5, and TS2769 at 22:14 and 22:21. With the directive in its previous place (as on e0854801) the same check passes.
Requested change: move // @ts-expect-error - bad tags back to the line directly above tags: ['baz', 'qux'],. Nothing else needs to change.
There was a problem hiding this comment.
That is incorrect. The IDE will flag error the way you suggested.
There was a problem hiding this comment.
🤖 I am the Storybook contribution review bot. This message was posted automatically; a human maintainer makes the final decision.
Review phase: Architecture review (stage 1 of 3)We are trying a three-stage automated review: architecture, code quality, and runtime verification. This message states which stages ran and which were skipped. Messages are posted automatically, and a human maintainer makes the final decision. Please tell us whether the feedback and evidence are useful.
You're right, thanks for pushing back! I checked: the IDE's TypeScript (6.x) reports this error on the meta.story( line, so the placement I suggested gives an unused-directive error there. Our check-package script uses TypeScript 7 native, which reports it on the tag values instead. That's why the earlier multi-line version failed CI. Putting the call on one line satisfies both: check-package passes on 0a15182, and if I remove the @ts-expect-error, the bad tags are flagged again. So this is resolved, and your fix is better than my suggestion.
Architecture and code quality review both pass on 0a15182. Runtime verification (stage 3) hasn't run on this commit yet; it runs next. Thanks!
There was a problem hiding this comment.
🤖 I am the Storybook contribution review bot. This message was posted automatically; a human maintainer makes the final decision.
Review phase: Runtime verification (stage 3 of 3)We are trying a three-stage automated review: architecture, code quality, and runtime verification. This message states which stages ran and which were skipped. Messages are posted automatically, and a human maintainer makes the final decision. Please tell us whether the feedback and evidence are useful.
All three stages pass on 0a15182: architecture, code quality and runtime verification. Thanks for sticking with this through the rounds!
Runtime verification (base next at 7cbbd9f vs this PR):
- Bug on base: custom story
tagsresolve tostring[] & (…)[], andtags: ['|']offers no completions, through both.type<{ tags }>()anddefinePreviewAddon<{ tags }>. - Fixed here: the type is exactly the custom union, and completions offer
fooandbar. The defaultTag[]behaviour and strict-union rejection are unchanged. - Tests guard the fix: with the base
story.ts, the new assertions fail (TS2554 incsf-factories.test.tsanddefine-preview.test.ts). On this PR,check-packagepasses for core and react-vite, and the two test files pass in Vitest (16 tests).
Not checked: completions in a real editor or .stories.tsx file (measured with the TypeScript language service on .ts files), and tag autocompletion in renderers other than React. A maintainer makes the merge decision.
What I did
Changed
AddonTypes.tagstype fromTag[] | undefinedtounknown.The original
Tag[]type was picked up by renderer packages like@storybook/react-vite, which widened the type when combined with custom tag types (e.g.,Array<'foo' | 'bar'>), breaking IDE autocompletion. Usingunknownallows renderers/frameworks to fully define their own tag types without interference from the base type.Type change for release notes:
AddonTypes.tagsis nowunknown(wasTag[] | undefined). A custom tags type declared viadefinePreviewAddon<{ tags: ... }>()or.type<{ tags: ... }>()now passes through unchanged. Note: likeargs, an addon that declares a non-arraytagstype is no longer rejected bydefinePreviewAddon; storytagsfalls back toTag[]in that case.Tests: added exact-type assertions to the core
customize tags typesuite (they fail without this change) and a strict-union negative case (Array<'foo' | 'bar'>rejects'baz').Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This is a TypeScript type-only change. Verify by checking that custom tags get autocompletion in an IDE:
@storybook/react-vite, define a preview with custom tags via.type<{ tags: Array<'foo' | 'bar' | (string & {})> }>()tags: ['and verify IDE autocomplete suggests'foo'and'bar'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.tsDeclare whether manual QA will be needed for this PR during the next release, through
qa:neededorqa:skipMake 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>