CSF-Next: Add tags type support for #34819
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMake story-level ChangesRenderer-aware tags customization
🎯 3 (Moderate) | ⏱️ ~20 minutes Comment |
|
Hi @unional Thank you very much for your contribution! Could you please complete the testing instructions with 1 or 2 scenarios so we can verify the feature ourselves? |
valentinpalkovic
moved this from Empathy Queue (prioritized)
to On Hold
in Core Team Projects
Hi @valentinpalkovic, the change is type related and the scenarios are in the unit tests added. What testing instructions do you need? You should see the autocompletion are working in the tests. |
valentinpalkovic
changed the title
|
Hi @unional, The build is failing. Could you please take a look? |
the looser `unknown[]` type doesn't work well with Generic + conditional type logic
fixed by strengthening the type from |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/core/src/csf/csf-factories.test.ts`:
- Around line 81-124: Add element-level "literal remainder" assertions to ensure
tags aren't widened to string[]: for both Parameters<typeof
preview.meta>[0]['tags'] and for the tags type on meta.story, extract the
element type (e.g. Elem = ...['tags'][number]), compute Remainder =
Exclude<Elem, string>, and add reciprocal testType.canAssign assertions
asserting 'foo' | 'bar' canAssign Remainder and Remainder canAssign 'foo' |
'bar' to catch widening; update the existing tests around preview.meta and
meta.story accordingly (refer to preview.meta, meta.story, and
testType.canAssign in the file).
In `@code/core/src/csf/story.ts`:
- Line 417: The conditional type for tags loses addon-provided tuple/element
types because AddonTypes.tags is optional, so TRenderer['tags'] can be
undefined; update the type expression that references TRenderer['tags'] so it
first removes undefined (e.g. via NonNullable or Exclude) before testing extends
string[] — i.e., change the check using TRenderer['tags'] to use
NonNullable<TRenderer['tags']> (or Exclude<TRenderer['tags'], undefined>) so
that the conditional preserves renderer/custom tuple types instead of always
falling back to plain string[]; locate the occurrence that reads like "tags?:
(TRenderer['tags'] extends string[] ? TRenderer['tags'] : string[]) | undefined"
and replace the conditional accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2753e743-e25e-46d7-998b-27edc1ba38b0
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
code/core/package.jsoncode/core/src/csf/csf-factories.test.tscode/core/src/csf/story.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
code/core/src/csf/story.ts (1)
417-417:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winConditional type still doesn't preserve custom tags (duplicate issue)
This issue was flagged in a previous review and remains unresolved. Since
AddonTypes.tagsis optional,TRenderer['tags']includesundefined, causingTRenderer['tags'] extends Tag[]to always be false and fall back toTag[], which defeats the purpose of this PR.The suggested fix from the previous review is correct, with a minor adjustment to use
Tag[]instead ofstring[]for consistency with line 188:Suggested fix
- tags?: (TRenderer['tags'] extends Tag[] ? TRenderer['tags'] : Tag[]) | undefined; + tags?: NonNullable<TRenderer['tags']> extends Tag[] + ? NonNullable<TRenderer['tags']> + : Tag[];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/core/src/csf/story.ts` at line 417, The conditional for the tags property fails because TRenderer['tags'] is optional (includes undefined); replace the check with a NonNullable-based conditional so undefined is stripped before the extends test and the custom tag type is preserved. Concretely, update the declaration using NonNullable<TRenderer['tags']> in the conditional (e.g. tags?: (NonNullable<TRenderer['tags']> extends Tag[] ? NonNullable<TRenderer['tags']> : Tag[]) | undefined) so TRenderer custom tag types are retained while still falling back to Tag[].
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@code/core/src/csf/story.ts`:
- Line 417: The conditional for the tags property fails because
TRenderer['tags'] is optional (includes undefined); replace the check with a
NonNullable-based conditional so undefined is stripped before the extends test
and the custom tag type is preserved. Concretely, update the declaration using
NonNullable<TRenderer['tags']> in the conditional (e.g. tags?:
(NonNullable<TRenderer['tags']> extends Tag[] ? NonNullable<TRenderer['tags']> :
Tag[]) | undefined) so TRenderer custom tag types are retained while still
falling back to Tag[].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8370fae1-4c0f-4514-b31c-03cb5500a221
📒 Files selected for processing (1)
code/core/src/csf/story.ts
github-project-automation
Bot
moved this from In Progress
to Done
in Core Team Projects
2 participants
Closes #34818
What I did
Adds ability to customize
tagstype for CSF Next.Note that I add
type-plusas dev dependency to test the type.Could not find a good way to test it otherwise.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
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
Tests
Chores