UI: Fix global background + color styles affecting stories#34071
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaced global/universal CSS selectors in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/core/assets/server/base-preview-head.html (1)
117-120: Add a regression check against the post-processed asset, not just the source CSS.This bug only showed up after downstream CSS processing, so it would be worth adding a fixture/snapshot/assertion on the built preview asset that fails if a top-level
* { background: ...; color: ... }rule reappears or if.sb-errordisplay_codeloses its scoped styling. That would make this fix much harder to regress silently.Also applies to: 157-160, 201-226
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/assets/server/base-preview-head.html` around lines 117 - 120, Add a regression test that asserts against the post-processed built preview asset (not the source CSS) to ensure the top-level universal rule and scoping regressions do not reappear: build or load the final preview bundle/asset produced from base-preview-head.html and assert that there is no top-level "* { background: rgb(247, 247, 247); color: rgb(46, 52, 56); }" rule present and that the ".sb-errordisplay_code" selector retains its scoped styles (i.e., its expected rules exist in the processed CSS). Locate the preview output artifact for the preview HTML/CSS (the built preview asset), run a snapshot or string/assertion check against it, and add this test alongside existing preview tests so future downstream CSS processing changes fail the test if the universal selector or unscoped styles reappear.
🤖 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/core/assets/server/base-preview-head.html`:
- Around line 157-160: The global selector .sb-errordisplay_main * is forcing a
white background onto all descendants, which overrides ANSI-colored spans
injected via innerHTML into `#error-stack` by preview-web/WebView.ts and breaks
the dark stack-trace block; adjust the CSS so the blanket reset excludes the
stack-trace subtree (e.g., exempt .sb-errordisplay_code and its descendants or
target only textual containers) so that <pre>/<code> inside
.sb-errordisplay_code keep their intended backgrounds and nested <span> colors
are not overwritten.
---
Nitpick comments:
In `@code/core/assets/server/base-preview-head.html`:
- Around line 117-120: Add a regression test that asserts against the
post-processed built preview asset (not the source CSS) to ensure the top-level
universal rule and scoping regressions do not reappear: build or load the final
preview bundle/asset produced from base-preview-head.html and assert that there
is no top-level "* { background: rgb(247, 247, 247); color: rgb(46, 52, 56); }"
rule present and that the ".sb-errordisplay_code" selector retains its scoped
styles (i.e., its expected rules exist in the processed CSS). Locate the preview
output artifact for the preview HTML/CSS (the built preview asset), run a
snapshot or string/assertion check against it, and add this test alongside
existing preview tests so future downstream CSS processing changes fail the test
if the universal selector or unscoped styles reappear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 63576d3c-5918-4e03-8a1a-45ea03a86e57
📒 Files selected for processing (1)
code/core/assets/server/base-preview-head.html
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
code/core/assets/server/base-preview-head.html (1)
208-234: Consider consolidating duplicate selector blocks.
.sb-errordisplay_main .sb-errordisplay_codeis declared twice (lines 208-225 and 232-234). While functionally correct, merging them improves maintainability.♻️ Suggested consolidation
.sb-errordisplay_main .sb-errordisplay_code { padding: 10px; flex: 1; background: `#242424`; color: `#c6c6c6`; box-sizing: border-box; font-size: 14px; font-weight: 400; line-height: 19px; border-radius: 4px; font-family: 'Operator Mono', 'Fira Code Retina', 'Fira Code', 'FiraCode-Retina', 'Andale Mono', 'Lucida Console', Consolas, Monaco, monospace; margin: 0; overflow: auto; + white-space: pre-wrap; } .sb-errordisplay_main .sb-errordisplay_code code { background-color: inherit; color: inherit; } - - .sb-errordisplay_main .sb-errordisplay_code { - white-space: pre-wrap; - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/assets/server/base-preview-head.html` around lines 208 - 234, The selector .sb-errordisplay_main .sb-errordisplay_code is defined twice; consolidate by merging all properties (padding, flex, background, color, box-sizing, font-size, font-weight, line-height, border-radius, font-family, margin, overflow and white-space) into a single .sb-errordisplay_main .sb-errordisplay_code rule and keep the nested .sb-errordisplay_main .sb-errordisplay_code code rule as-is, then remove the duplicate empty/overlapping block to improve maintainability.
🤖 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/core/assets/server/base-preview-head.html`:
- Around line 170-177: In .sb-errordisplay_main h1 update the font-family to
quote the multi-word family (e.g., "Nunito Sans") and remove the duplicate
font-weight declaration so only one font-weight (either 400 or normal) remains;
locate the .sb-errordisplay_main h1 rule in base-preview-head.html and make
those two edits.
- Around line 189-196: The CSS rule for .sb-errordisplay_main p and
.sb-errordisplay_main ol uses an unquoted multi-word font family "Nunito Sans";
update the font-family declaration in that rule to quote the multi-word name
(e.g., "Nunito Sans") so the browser parses it correctly and matches the
existing h1 fix pattern.
---
Nitpick comments:
In `@code/core/assets/server/base-preview-head.html`:
- Around line 208-234: The selector .sb-errordisplay_main .sb-errordisplay_code
is defined twice; consolidate by merging all properties (padding, flex,
background, color, box-sizing, font-size, font-weight, line-height,
border-radius, font-family, margin, overflow and white-space) into a single
.sb-errordisplay_main .sb-errordisplay_code rule and keep the nested
.sb-errordisplay_main .sb-errordisplay_code code rule as-is, then remove the
duplicate empty/overlapping block to improve maintainability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa397a4f-c788-4356-8370-773fc1f6e13c
📒 Files selected for processing (1)
code/core/assets/server/base-preview-head.html
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/core/assets/server/base-preview-head.html`:
- Around line 170-171: The CSS for .sb-errordisplay_main h1 (and similarly for p
and ol) overrides the intended .sb-wrapper font stack with only 'Nunito Sans';
change the rule to either inherit the wrapper stack or repeat the full fallback
system sans stack instead of a single font so browsers fall back correctly when
'Nunito Sans' is unavailable — update the selectors .sb-errordisplay_main h1,
.sb-errordisplay_main p, and .sb-errordisplay_main ol to use font-family:
inherit; or font-family: 'Nunito Sans', -apple-system, BlinkMacSystemFont,
"Segoe UI", Roboto, "Helvetica Neue", Arial, "Noto Sans", "Apple Color Emoji",
"Segoe UI Emoji", "Segoe UI Symbol";
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8284b08c-ec3a-4e2e-bb82-7b3d351034da
📒 Files selected for processing (1)
code/core/assets/server/base-preview-head.html
|
I've used this code in my project and is working as expected. can it be merged asap please ? |
|
@valentinpalkovic ping |
|
Hello, may I know when we can expect an update on this issue? |
There was a problem hiding this comment.
Thanks for the fix, LGTM. 🙏
It would be great if someone could provide an actual reproduction for this issue to verify against. A repository that clearly demonstrates the behavior.
During production builds, some CSS processors and minifiers used in common toolchains (e.g. Vite + PostCSS + cssnano/lightningcss) incorrectly flattened this nested rule into a global selector
I think this is correct, but I'd like to see which combination of tooling and configuration causes this, because if you just spin up a default Vite project with the latest versions, it handles CSS nesting just fine.
JReinhold
moved this from Empathy Queue (prioritized)
to In Progress
in Core Team Projects
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 186 | 186 | 0 |
| Self size | 76 KB | 76 KB | 0 B |
| Dependency size | 32.78 MB | 32.80 MB | 🚨 +13 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/ember
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 190 | 190 | 0 |
| Self size | 15 KB | 15 KB | 0 B |
| Dependency size | 29.50 MB | 29.51 MB | 🚨 +13 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 535 | 535 | 0 |
| Self size | 650 KB | 650 KB | 0 B |
| Dependency size | 60.80 MB | 60.81 MB | 🚨 +13 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 273 | 273 | 0 |
| Self size | 23 KB | 23 KB | 0 B |
| Dependency size | 45.41 MB | 45.42 MB | 🚨 +13 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/server-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 198 | 198 | 0 |
| Self size | 16 KB | 16 KB | 0 B |
| Dependency size | 34.04 MB | 34.06 MB | 🚨 +13 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/preset-react-webpack
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 166 | 166 | 0 |
| Self size | 18 KB | 18 KB | 🎉 -24 B 🎉 |
| Dependency size | 31.78 MB | 31.79 MB | 🚨 +13 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
JReinhold
changed the title
github-project-automation
Bot
moved this from In Progress
to Done
in Core Team Projects
JReinhold
added
the
patch:yes
github-actions
Bot
added
the
patch:done
5 participants
Summary
Refactor the CSS in
base-preview-head.htmlto remove nested selectors and replace them with flat, fully-qualified selectors to prevent style leakage during CSS post-processing. Additionally, correct the selector used for styling the error stack<pre>element so it matches the actual markup structure.Root Cause
The error display styles previously relied on nested CSS syntax such as:
During production builds, some CSS processors and minifiers used in common toolchains (e.g. Vite + PostCSS + cssnano/lightningcss) incorrectly flattened this nested rule into a global selector:
This caused the rule to escape the
.sb-errordisplay_mainscope and override styles of all components rendered inside the Storybook preview iframe.A similar nesting pattern existed in
.sb-nopreview_mainusing rules like& *, which could also be flattened incorrectly by certain CSS processors.Additionally, one selector introduced during the refactor targeted:
However, the actual markup defined in
base-preview-body.htmlrenders the stack trace as:directly under
.sb-errordisplay_main. As a result, the selector did not match the DOM and the intended white-space styling was not applied.Fix
This change removes all nested CSS selectors and replaces them with explicit flat selectors to ensure compatibility with all CSS processing pipelines.
Examples of the changes include:
.sb-nopreview_mainnested rules with flat equivalents:Using flat selectors ensures that styles remain properly scoped and prevents build tools from incorrectly hoisting selectors to the global scope.
Testing
This change affects a static HTML/CSS asset and does not have associated unit tests. The fix can be validated by:
* { background: ... }rule appears..sb-errordisplay_mainor.sb-nopreview_main.Closes #34036
Closes #33947
Closes #33735
Summary by CodeRabbit