Conversation
There was a problem hiding this comment.
Pull request overview
This PR expands the docs Storybook with additional interactive “Examples” stories that demonstrate performance/debugging concepts (element timing, memory pressure, style churn, and memoization behavior), and updates Storybook preview parameters to enable docs code panels.
Changes:
- Added new interactive demo stories: Element Timing, Memory Pressure, Style Churn, and Memoization Waste.
- Updated Storybook preview parameters to enable a docs code panel.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/docs/stories/StyleChurn.stories.tsx | New story demonstrating heavy style writes and DOM churn. |
| packages/docs/stories/MemoryPressure.stories.tsx | New story demonstrating allocation-driven memory/GC pressure. |
| packages/docs/stories/MemoizationWaste.stories.tsx | New story demonstrating wasted renders vs effective memoization. |
| packages/docs/stories/ElementTiming.stories.tsx | New story demonstrating the Element Timing API via elementtiming attributes. |
| packages/docs/.storybook/preview.ts | Enables docs code panel in Storybook parameters. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const startChurn = useCallback(() => { | ||
| if (timerRef.current) return | ||
| setRunning(true) | ||
| // Rapid-fire style mutations every frame | ||
| timerRef.current = setInterval(mutateBurst, 16) | ||
| }, [mutateBurst]) | ||
|
|
||
| const stopChurn = useCallback(() => { | ||
| if (timerRef.current) { | ||
| clearInterval(timerRef.current) | ||
| timerRef.current = null | ||
| } | ||
| setRunning(false) | ||
| }, []) |
There was a problem hiding this comment.
setInterval is started in startChurn, but there's no unmount cleanup. If the user navigates away from the story while running, the interval will keep firing and can mutate a detached DOM node. Add a useEffect cleanup that clears the interval (or call stopChurn in a cleanup) so the demo can’t leak work across story switches.
| const addRemoveNodes = useCallback(() => { | ||
| const container = containerRef.current | ||
| if (!container) return | ||
|
|
||
| // DOM churn: remove half the nodes and re-add new ones | ||
| const cells = container.querySelectorAll('[data-cell]') | ||
| const toRemove = Math.floor(cells.length / 2) | ||
| for (let i = 0; i < toRemove; i++) { | ||
| cells[i]?.remove() | ||
| } | ||
| for (let i = 0; i < toRemove; i++) { | ||
| const div = document.createElement('div') | ||
| div.setAttribute('data-cell', '') | ||
| const hue = Math.floor(Math.random() * 360) | ||
| div.style.cssText = `width:24px;height:24px;background:hsl(${String(hue)},70%,80%);border-radius:2px;transition:all 0.15s` | ||
| container.appendChild(div) | ||
| } | ||
| setPhase(p => p + 1) | ||
| }, []) |
There was a problem hiding this comment.
addRemoveNodes directly removes/appends DOM children that are also created by React in the component’s render. This can put React’s reconciliation out of sync with the actual DOM (especially since setPhase forces a re-render right after), leading to unpredictable behavior. Prefer driving the node list via React state (e.g., an array of cell ids) to remove/add items, or move the churned nodes into an imperatively-managed subtree that React does not render into.
| const startAllocating = useCallback(() => { | ||
| if (timerRef.current) return | ||
| setRunning(true) | ||
| timerRef.current = setInterval(allocate, 50) | ||
| }, [allocate]) | ||
|
|
||
| const stopAllocating = useCallback(() => { | ||
| if (timerRef.current) { | ||
| clearInterval(timerRef.current) | ||
| timerRef.current = null | ||
| } | ||
| setRunning(false) | ||
| }, []) |
There was a problem hiding this comment.
setInterval is started in startAllocating, but there’s no cleanup on unmount. If the user switches stories while allocations are running, the interval will continue allocating in the background. Add a useEffect cleanup that clears the interval (or reuse stopAllocating in a cleanup) to prevent background work leaking across story navigation.
| // The bug: creating a new config object every render defeats React.memo | ||
| const config = stableConfig ? {highlight: true} : {highlight: true} | ||
| // In the "stable" variant we use a module-level constant | ||
| const configProp = stableConfig ? STABLE_CONFIG : config | ||
|
|
There was a problem hiding this comment.
The config creation is redundant and misleading: stableConfig ? {highlight: true} : {highlight: true} always allocates a new object, and in the stable variant it allocates an unused object every render. For clarity (and to better demonstrate the memoization point), only create a new object in the broken variant, and use STABLE_CONFIG (or a useMemo) in the stable variant without allocating the unused object.
| // The bug: creating a new config object every render defeats React.memo | |
| const config = stableConfig ? {highlight: true} : {highlight: true} | |
| // In the "stable" variant we use a module-level constant | |
| const configProp = stableConfig ? STABLE_CONFIG : config | |
| // In the "stable" variant we use a module-level constant; in the "broken" variant we | |
| // create a new config object every render to defeat React.memo. | |
| const configProp = stableConfig ? STABLE_CONFIG : {highlight: true} |
- Add useEffect cleanup for setInterval in StyleChurn and MemoryPressure to prevent leaked intervals on unmount/story switches - Convert DOM churn in StyleChurn from imperative DOM manipulation to React-state-driven rendering to keep reconciliation in sync - Simplify redundant config allocation in MemoizationWaste to only allocate a new object in the broken variant
|
@mattcosta7 quick question on pull metrics support for Web Components stories. From what I can tell, it doesn’t appear to be compatible out of the box. When I attempted to integrate it, the metrics layer wasn’t able to resolve the story correctly, so I ended up prototyping a lightweight wrapper to bridge the gap gist linked for context Before we formalize that approach, do you see a more idiomatic way to enable metrics for Web Components stories? I’d prefer to avoid maintaining custom glue code if there’s a supported extension point we can leverage. |
Mind filing an issue for adding support for non-react storybooks? Happy to take a look at that next week to see how we can architect in support across platforms. We started with react support only because that's what we're using internally, but I think we can come up with some simpler integration points Happy to take ideas on that too if you have any thoughts! |
actually - maybe something like this where we just lift things out of react, and only use the react bits where necessary this still has the react export as the main one, with the non-react as a subpath, but we can flip that later on potentially - assuming this makes sense |
|
@mattcosta7 Loved the architecture and direction. Some suggestions and points to discuss left on the PR. Would be really helpful if we can bake it in as a |
Summary
Adds a set of interactive Storybook example stories to make it easier to see what the Performance Panel is measuring in real time. These stories intentionally create observable signals (render work, style writes, memory/GC churn, and Element Timing marks) so users can validate the addon is installed/configured correctly and learn how to interpret the panel’s metrics.
Also enables Storybook Docs code panels for the docs package.
What’s included
New demo stories (interactive)
Examples/Element Timing
elementtimingattribute to<img>elements (set imperatively since React doesn’t recognize it as a standard DOM prop).Examples/Memory Pressure
Examples/Style Churn
Examples/Memoization Waste
Docs config tweak
codePanelin the docs package preview config so examples can be inspected alongside the rendered output.Files changed
packages/docs/.storybook/preview.tspackages/docs/stories/ElementTiming.stories.tsx(new)packages/docs/stories/MemoryPressure.stories.tsx(new)packages/docs/stories/StyleChurn.stories.tsx(new)packages/docs/stories/MemoizationWaste.stories.tsx(new)Testing / How to verify
Notes
StyleChurn,MemoryPressure) include unmount cleanup to prevent leaked intervals across story navigation.