-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(stories): refactor story layout #94009
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
d734ebe
to
e8960e9
Compare
c1562c4
to
dd8abf7
Compare
a930898
to
b9858a2
Compare
05060be
to
5d0f669
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eab25fa
to
67be9a4
Compare
5d0f669
to
645d135
Compare
67be9a4
to
2670743
Compare
645d135
to
e652c51
Compare
e07c3ba
to
dc99486
Compare
82df4d4
to
165e2af
Compare
OK this PR has gotten massive so I'm going to merge what's here and have another branch coming with search fixes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Ref Callback Cleanup Causes Memory Leak
The ref callback for the search input incorrectly returns a cleanup function. Unlike useEffect
, React ref callbacks do not support cleanup functions; the returned function is ignored. This prevents the focus
event listener from being properly removed, leading to memory leaks.
static/app/stories/view/storySearch.tsx#L63-L72
sentry/static/app/stories/view/storySearch.tsx
Lines 63 to 72 in 857972d
</InputGroup.LeadingItems> | |
<InputGroup.Input | |
ref={el => { | |
searchInput.current = el; | |
const handleFocus = () => setShowSearch(true); | |
el?.addEventListener('focus', handleFocus); | |
return () => { | |
el?.removeEventListener('focus', handleFocus); | |
}; | |
}} |
Bug: Error Logging Masks Potential Bugs
The console.error
statement in StoryUsage
was accidentally committed. It replaces a throw new Error
, causing unsupported story exports to be logged to the console instead of failing fast. This masks potential bugs and will log errors in production.
static/app/stories/view/storyExports.tsx#L136-L141
sentry/static/app/stories/view/storyExports.tsx
Lines 136 to 141 in 857972d
} | |
// eslint-disable-next-line no-console | |
console.error( | |
`Story exported an unsupported key ${name} with value: ${typeof MaybeComponent}` | |
); | |
return null; |
Was this report helpful? Give feedback by reacting with 👍 or 👎
Final changes to make #94005 backwards-compatible
Final changes to make #94005 backwards-compatible
Final changes to make #94005 backwards-compatible
Final changes to make #94005 backwards-compatible
Final changes to make #94005 backwards-compatible