Core: Avoid hanging when inferring args for recursive calls on DOM elemens#33922
Merged
valentinpalkovic merged 1 commit intoFeb 25, 2026
Merged
Conversation
…calls on DOM elemens
Contributor
valentinpalkovic
changed the title
|
View your CI Pipeline Execution ↗ for commit 8e1bafb
☁️ Nx Cloud last updated this comment at |
Contributor
📝 WalkthroughWalkthroughAdds caching and cycle-aware traversal to type inference in 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
code/core/src/preview-api/modules/store/inferArgTypes.ts (2)
59-60: Consider caching before removing fromvisitedfor defensive correctness.The current order —
visited.deletethencache.set— is safe today becausemapValueson line 55 is fully synchronous. However, the reverse order (cache.setfirst, thenvisited.delete) would be more defensively correct: a value in cache already has a complete result, so any re-entry would short-circuit cleanly. The current order creates a momentary window (between the two lines) where the value is neither invisitednor incache, which could cause redundant recomputation if this code ever becomes async.♻️ Suggested defensive ordering
- visited.delete(value); // Remove from current path after processing - cache.set(value, result); // Cache the result for future lookups + cache.set(value, result); // Cache the result before removing from visited path + visited.delete(value); // Remove from current path after processing🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/preview-api/modules/store/inferArgTypes.ts` around lines 59 - 60, Change the ordering when finalizing a computed result so the value is inserted into the cache before it is removed from the recursion guard: call cache.set(value, result) prior to visited.delete(value). This change in the logic inside the function performing the mapValues traversal (the code around mapValues and the visited/cache handling) ensures a fully computed result is visible to re-entrancy checks and avoids the transient window where a value is in neither visited nor cache, making the routine safe if it becomes asynchronous.
67-73: The asymmetry between sharedcacheand per-argnew Set()is intentional and correct.
cacheis shared across all top-level args, enabling cross-arg memoization for shared object references.visitedis fresh per arg so that a diamond-shaped reference (object appearing in two non-cyclic positions) is not falsely flagged as a cycle.One consequence worth documenting: a cyclic object encountered in the first arg is cached with sentinel fields. Subsequent args referencing the same object return the cached result silently (no repeated warning). This is correct behavior — warn once — but worth noting in a code comment for future maintainers.
📝 Optional documentation comment
const cache = new Map<any, SBType>(); + // `cache` is shared across all args for cross-arg memoization. A fresh `visited` Set per + // arg prevents false cycle detection for shared (non-cyclic) object references. const argTypes = mapValues(initialArgs, (arg, key) => ({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/preview-api/modules/store/inferArgTypes.ts` around lines 67 - 73, Add a short explanatory comment in inferArgTypes next to the shared cache/new Set() usage explaining that cache (Map named cache) is intentionally shared across all top-level args to enable cross-arg memoization (so repeated references return cached SBType), while the per-arg visited set (new Set() passed into inferType) is freshly created for each arg to avoid false cycle detection for diamond-shaped references; also note the consequence that a cyclic object first encountered is cached with sentinel fields and subsequent references will use the cached result silently (warn once).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/core/src/preview-api/modules/store/inferArgTypes.ts`:
- Around line 59-60: Change the ordering when finalizing a computed result so
the value is inserted into the cache before it is removed from the recursion
guard: call cache.set(value, result) prior to visited.delete(value). This change
in the logic inside the function performing the mapValues traversal (the code
around mapValues and the visited/cache handling) ensures a fully computed result
is visible to re-entrancy checks and avoids the transient window where a value
is in neither visited nor cache, making the routine safe if it becomes
asynchronous.
- Around line 67-73: Add a short explanatory comment in inferArgTypes next to
the shared cache/new Set() usage explaining that cache (Map named cache) is
intentionally shared across all top-level args to enable cross-arg memoization
(so repeated references return cached SBType), while the per-arg visited set
(new Set() passed into inferType) is freshly created for each arg to avoid false
cycle detection for diamond-shaped references; also note the consequence that a
cyclic object first encountered is cached with sentinel fields and subsequent
references will use the cached result silently (warn once).
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 92 | 92 | 0 |
| Self size | 1.12 MB | 1.12 MB | 🎉 -36 B 🎉 |
| Dependency size | 22.39 MB | 22.46 MB | 🚨 +72 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 124 | 124 | 0 |
| Self size | 30 KB | 30 KB | 0 B |
| Dependency size | 23.68 MB | 23.75 MB | 🚨 +72 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 82 | 82 | 0 |
| Self size | 35 KB | 35 KB | 0 B |
| Dependency size | 20.17 MB | 20.25 MB | 🚨 +72 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/vue3-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 108 | 108 | 0 |
| Self size | 35 KB | 36 KB | 🚨 +48 B 🚨 |
| Dependency size | 43.74 MB | 43.77 MB | 🚨 +33 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
eslint-plugin-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 20 | 20 | 0 |
| Self size | 131 KB | 131 KB | 0 B |
| Dependency size | 3.42 MB | 3.50 MB | 🚨 +72 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
valentinpalkovic
added
the
patch:yes
valentinpalkovic
deleted the
valentin/fix-performance-in-infer-arg-types
branch
9 tasks
3 tasks
8 tasks
yannbf
pushed a commit
that referenced
this pull request
Feb 26, 2026
…n-infer-arg-types Core: Avoid hanging when inferring args for recursive calls on DOM elemens (cherry picked from commit bbca9fc)
github-actions
Bot
added
the
patch:done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #33821
Closes #17098
Closes #19575
Closes #28750
Closes #17482
Closes #16855
What I did
The problem (thanks @sonsu-lee for the investigations!):
Fix
Unlike in the original PR, which attempts to solve the issue by being very conservative about which data types to support, I have used a
Mapto cache the inferred results and share the samevisitedset across all branches, reducing theamount of warning messages in scenarios, where e.g. plain class instances may be allowed as args.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 pull request has been released as version
0.0.0-pr-33922-sha-8e1bafbc. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-33922-sha-8e1bafbc sandboxor in an existing project withnpx storybook@0.0.0-pr-33922-sha-8e1bafbc upgrade.More information
0.0.0-pr-33922-sha-8e1bafbcvalentin/fix-performance-in-infer-arg-types8e1bafbc1772012440)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=33922Summary by CodeRabbit
Bug Fixes
Performance