Addon-Docs: Avoid rerendering static Source blocks#34206
Merged
Sidnioulz merged 11 commits intoApr 13, 2026
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAdds a Profiler-based benchmark Story and tests for Source, treats explicit empty Changes
Sequence Diagram(s)sequenceDiagram
participant BH as BenchmarkHarness
participant Prof as ReactProfiler
participant Src as Source
participant DOM as BrowserPaint
BH->>Prof: mount StaticSourceList (N blocks)
Prof->>Src: onRender callback (record render)
Src-->>Prof: render
Prof->>DOM: paint
BH->>BH: start benchmark (5 iterations)
loop 5x
BH->>BH: update iteration key (force re-mount)
BH->>DOM: waitForNextPaint()
BH->>Prof: remount StaticSourceList (new key)
Prof->>Src: onRender callback (increment)
Prof->>DOM: paint
end
BH->>BH: compute elapsed time & extra renders
BH->>DOM: update result UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
code/addons/docs/src/blocks/blocks/Source.test.tsx (2)
57-139: Please add one regression for the newInvalidBlockOfPropError.This suite locks in the rerender split, but the typed error introduced on Line 141 of
Source.tsxstill has no automated coverage. A smalltoThrow(InvalidBlockOfPropError)case would keep that contract from regressing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/addons/docs/src/blocks/blocks/Source.test.tsx` around lines 57 - 139, Add a regression test in Source.test.tsx that asserts the new typed error is thrown: render a Source block (e.g., StorySource or StaticSource as appropriate) with an invalid "of" prop value that triggers the InvalidBlockOfPropError from Source.tsx and wrap the render in an expect(...).toThrow(InvalidBlockOfPropError). Use the existing test helpers (createMockDocsContext, render) and the InvalidBlockOfPropError symbol to ensure the test specifically checks for that error type so the contract cannot regress.
16-25: Please align this module mock with the repo's Vitest spy pattern.Fully replacing
../components/Sourcehere diverges from thespy: true+vi.mocked()+beforeEachconvention and forces this test to hand-maintain the mocked module shape. Spying on the real module would keepSourceErrorin sync while still letting you capture props.As per coding guidelines, "Use `vi.mock()` with the `spy: true` option for all package and file mocks in Vitest tests" and "Implement mock behaviors in `beforeEach` blocks in Vitest tests".♻️ Suggested update
import React, { memo } from 'react'; +import * as SourceComponentModule from '../components/Source'; import type { PreparedStory } from 'storybook/internal/types'; @@ const pureSourceSpy = vi.fn(); -vi.mock('../components/Source', () => ({ - Source: (props: unknown) => { - pureSourceSpy(props); - return null; - }, - SourceError: { - NO_STORY: 'There’s no story here.', - SOURCE_UNAVAILABLE: 'Oh no! The source is not available.', - }, -})); +vi.mock('../components/Source', { spy: true }); @@ describe('Source', () => { beforeEach(() => { pureSourceSpy.mockClear(); + vi.mocked(SourceComponentModule.Source).mockImplementation((props: unknown) => { + pureSourceSpy(props); + return null; + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/addons/docs/src/blocks/blocks/Source.test.tsx` around lines 16 - 25, Replace the full-module replacement of '../components/Source' with a spy-style mock: call vi.mock('../components/Source', { spy: true }) so the real module is loaded and SourceError remains intact, then in a beforeEach use vi.mocked(...) to override only the Source export with a wrapper that calls pureSourceSpy(props) and returns null; reference the exports Source and SourceError and use pureSourceSpy and vi.mocked to implement the per-test mock behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/addons/docs/src/blocks/blocks/Source.tsx`:
- Around line 190-193: SourceImpl currently branches on truthiness of props.code
which treats an empty string as absent and keeps the component subscribed to
SourceContext; change the conditional to check for explicit undefined (e.g.,
props.code !== undefined) so a provided empty string uses SourceWithCode; mirror
the same explicit undefined checks inside useSourceProps and any guards so they
choose the code path (and avoid SourceContext subscription) whenever props.code
is present, and continue to use SourceWithStorySnippet only when props.code is
strictly undefined.
---
Nitpick comments:
In `@code/addons/docs/src/blocks/blocks/Source.test.tsx`:
- Around line 57-139: Add a regression test in Source.test.tsx that asserts the
new typed error is thrown: render a Source block (e.g., StorySource or
StaticSource as appropriate) with an invalid "of" prop value that triggers the
InvalidBlockOfPropError from Source.tsx and wrap the render in an
expect(...).toThrow(InvalidBlockOfPropError). Use the existing test helpers
(createMockDocsContext, render) and the InvalidBlockOfPropError symbol to ensure
the test specifically checks for that error type so the contract cannot regress.
- Around line 16-25: Replace the full-module replacement of
'../components/Source' with a spy-style mock: call
vi.mock('../components/Source', { spy: true }) so the real module is loaded and
SourceError remains intact, then in a beforeEach use vi.mocked(...) to override
only the Source export with a wrapper that calls pureSourceSpy(props) and
returns null; reference the exports Source and SourceError and use pureSourceSpy
and vi.mocked to implement the per-test mock behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ef494ab-2921-45b8-8d97-78700c833dc6
📒 Files selected for processing (4)
code/addons/docs/src/blocks/blocks/Source.stories.tsxcode/addons/docs/src/blocks/blocks/Source.test.tsxcode/addons/docs/src/blocks/blocks/Source.tsxcode/core/src/preview-errors.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/addons/docs/src/blocks/blocks/Source.stories.tsx (1)
133-139: Minor: Redundant DOM property assignment.Setting both
.valueand.textContenton the<output>element is redundant—.textContentwill display the value regardless. However, this doesn't affect functionality.Optional simplification
const updateCount = useCallback((value: number) => { renderCount.current = value; if (countRef.current) { - countRef.current.value = `${value}`; countRef.current.textContent = `${value}`; } }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/addons/docs/src/blocks/blocks/Source.stories.tsx` around lines 133 - 139, In updateCount (the useCallback that updates renderCount and uses countRef), remove the redundant DOM assignment to countRef.current.value on the <output> element and only set countRef.current.textContent = `${value}` (or alternatively only set .value) to simplify the code; keep renderCount.current update and the null check for countRef.current intact and update any related comments to reflect the single property being used.
🤖 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/addons/docs/src/blocks/blocks/Source.stories.tsx`:
- Around line 133-139: In updateCount (the useCallback that updates renderCount
and uses countRef), remove the redundant DOM assignment to
countRef.current.value on the <output> element and only set
countRef.current.textContent = `${value}` (or alternatively only set .value) to
simplify the code; keep renderCount.current update and the null check for
countRef.current intact and update any related comments to reflect the single
property being used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce3bad83-ec70-47fd-a4d2-28b3936c4285
📒 Files selected for processing (1)
code/addons/docs/src/blocks/blocks/Source.stories.tsx
JReinhold
requested changes
Mar 25, 2026
Contributor
JReinhold
left a comment
JReinhold
left a comment
There was a problem hiding this comment.
Thanks for the PR! The idea looks good, added a few remarks on the specifics.
JReinhold
moved this from Empathy Queue (prioritized)
to In Progress
in Core Team Projects
anchmelev
force-pushed
the
fix/23776-source-static-rerenders
branch
from
5c35882 to
0472750
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/addons/docs/src/blocks/blocks/Source.stories.tsx (1)
292-296: Consider adding a play function for automated verification.The
!vitesttag disables automated testing, making this purely manual. Sincedata-testidattributes are already present, a play function could automate the verification:play: async ({ canvasElement }) => { const canvas = within(canvasElement); const runButton = await canvas.findByTestId('run-benchmark'); await userEvent.click(runButton); const result = await canvas.findByTestId('benchmark-result'); await waitFor(() => { expect(result.textContent).toMatch(/^0 extra renders/); }); },This would catch future regressions automatically. However, if the intent is to keep this as a manual performance inspection tool only (with functional coverage in
Source.test.tsx), the current approach is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/addons/docs/src/blocks/blocks/Source.stories.tsx` around lines 292 - 296, Add an automated play function to the ManyStaticCodeBlocksBenchmark story so the benchmark is executed and verified in Storybook tests: in the ManyStaticCodeBlocksBenchmark object (which currently calls <BenchmarkHarness blocks={75} />) add a play: async ({ canvasElement }) => { const canvas = within(canvasElement); const runButton = await canvas.findByTestId('run-benchmark'); await userEvent.click(runButton); const result = await canvas.findByTestId('benchmark-result'); await waitFor(() => expect(result.textContent).toMatch(/^0 extra renders/)); } using the existing data-testid attributes ('run-benchmark' and 'benchmark-result'); keep or adjust the parameters/tags as desired (you can keep '!vitest' if you intentionally want to skip other test suites).
🤖 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/addons/docs/src/blocks/blocks/Source.stories.tsx`:
- Around line 292-296: Add an automated play function to the
ManyStaticCodeBlocksBenchmark story so the benchmark is executed and verified in
Storybook tests: in the ManyStaticCodeBlocksBenchmark object (which currently
calls <BenchmarkHarness blocks={75} />) add a play: async ({ canvasElement }) =>
{ const canvas = within(canvasElement); const runButton = await
canvas.findByTestId('run-benchmark'); await userEvent.click(runButton); const
result = await canvas.findByTestId('benchmark-result'); await waitFor(() =>
expect(result.textContent).toMatch(/^0 extra renders/)); } using the existing
data-testid attributes ('run-benchmark' and 'benchmark-result'); keep or adjust
the parameters/tags as desired (you can keep '!vitest' if you intentionally want
to skip other test suites).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc0ba5f9-d15e-4604-975f-73682e3d936c
📒 Files selected for processing (4)
code/addons/docs/src/blocks/blocks/Source.stories.tsxcode/addons/docs/src/blocks/blocks/Source.test.tsxcode/addons/docs/src/blocks/blocks/Source.tsxcode/core/src/preview-errors.ts
✅ Files skipped from review due to trivial changes (1)
- code/addons/docs/src/blocks/blocks/Source.test.tsx
Contributor
Author
Thanks for the review - I really appreciate it. I went through the remarks, addressed them inline, and pushed follow-up changes. Please take another look when you have a chance. |
JReinhold
approved these changes
Mar 27, 2026
Contributor
Author
|
Hi @JReinhold, thanks again for the approval on this PR. |
github-project-automation
Bot
moved this from In Progress
to Done
in Core Team Projects
This was referenced Apr 13, 2026
8 tasks
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
4 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 #23776
What I did
Static
<Source code="...">blocks were subscribing toSourceContextupdates even though they do not depend on live story snippets. On docs pages with manySourceblocks, each snippet update could trigger a wave of unnecessary rerenders and repeated syntax-highlighting work.This change:
codesources use an empty source context and no longer rerender on unrelated snippet updatesSourceblocks reactive to snippet updatesErrorforof={undefined}with a typedStorybookErrorBenchmark
Local benchmark result:
<Source code="...">blocksHow to verify
cd code && yarn storybook:uiBlocks/Source/ManyStaticCodeBlocksBenchmarkRun 5 updates0 extra renders ...for staticSource codeblocksBlocks/Source/Ofand verify attached story source still behaves normallySummary by CodeRabbit
New Features
Bug Fixes
ofprop is present but explicitly undefined.Refactor
Tests