Core: Replace Emotion theme variables with CSS variables#33516
Core: Replace Emotion theme variables with CSS variables#33516ghengeveld wants to merge 9 commits into
Conversation
ghengeveld
force-pushed
the
css-theme-variables
branch
from
aa7c9a9 to
2e84f35
Compare
ghengeveld
changed the title
|
View your CI Pipeline Execution ↗ for commit 48c2d7b
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughMigrates most styled-component theme token usage to CSS custom properties (var(--sb-...)) across the codebase and adds a new ThemeVariables component that extracts runtime theme values into --sb-* CSS variables and injects them into Global styles. Changes
Sequence Diagram(s)sequenceDiagram
participant App as React App
participant ThemeProvider as ThemeProvider
participant ThemeVariables as ThemeVariables
participant useTheme as useTheme Hook
participant Global as Global Style
participant Components as Styled Components
App->>ThemeProvider: render children with theme
ThemeProvider->>ThemeVariables: mount inside provider
ThemeVariables->>useTheme: call useTheme()
useTheme-->>ThemeVariables: return theme object
ThemeVariables->>ThemeVariables: traverse theme -> build --sb-* map
ThemeVariables->>Global: inject CSS variables under rootSelector
Global-->>App: CSS vars available to DOM
App->>Components: styled components consume var(--sb-...) in CSS
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing touches
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
code/core/src/manager/settings/Checklist/Checklist.tsx (1)
59-71: MissingonClickin type definition.
onClickis destructured and used on line 71, but it's not included in the component's generic type<{ progress: number; isCollapsed: boolean }>. This will cause TypeScript errors with strict mode enabled. AddonClickto the type definition.Proposed fix
-const SectionSummary = styled.div<{ progress: number; isCollapsed: boolean }>( +const SectionSummary = styled.div<{ progress: number; isCollapsed: boolean; onClick?: () => void }>( ({ theme, progress, isCollapsed, onClick }) => ({code/core/src/components/components/Tabs/Tabs.hooks.tsx (1)
27-40: Bug: Extra quotes around CSS variable values in hover styles.Lines 34 and 36 have the CSS variable wrapped in extra quotes inside the template literal, which will produce invalid CSS like
color: 'var(--sb-barHoverColor)'instead ofcolor: var(--sb-barHoverColor).🐛 Proposed fix
&:hover { - color: 'var(--sb-barHoverColor)'; + color: var(--sb-barHoverColor); .addon-collapsible-icon { - color: 'var(--sb-barHoverColor)'; + color: var(--sb-barHoverColor); } }code/core/src/components/components/typography/link/link.tsx (1)
66-101: Incomplete migration: hover/active states still use theme tokens.The base
colorandfillproperties have been migrated to CSS variables (lines 72, 98), but the hover/active states (lines 74-86) still usedarken()withtheme.color.secondary. This creates an inconsistency where the base color comes from a CSS variable but interactive states are computed from the theme object.Consider completing the migration by introducing CSS variables for the hover/active states (e.g.,
--sb-color-secondary-hover,--sb-color-secondary-active) or keeping all states using the same source (either all theme or all CSS vars).
🤖 Fix all issues with AI agents
In @code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx:
- Around line 68-79: The styled component Wrapper (typed with WrapperProps) uses
the CSS variable var(--sb-borderRadius); change that to
var(--sb-appBorderRadius) so the bordered variant uses the project-standard
variable name; update the borderRadius property inside the bordered branch of
Wrapper to use var(--sb-appBorderRadius) to match other components like
Tabs.tsx/Card.tsx/Popover.tsx.
In @code/core/src/components/components/typography/DocumentWrapper.tsx:
- Around line 182-204: The table row style inside DocumentWrapper.tsx uses a
hardcoded backgroundColor 'white' in the '& tr' rule which breaks dark mode;
replace that hardcoded value with the theme-aware CSS variable (e.g. use
var(--sb-appBg) or your project's content background variable) so the base rows
adapt to dark mode while keeping the existing '&:nth-of-type(2n)' alternating
row rule that uses var(--sb-color-lighter).
In @code/core/src/manager/components/sidebar/FileList.tsx:
- Around line 198-205: The padding value in DefaultExport uses invalid CSS
`var(--sb-appBorderRadius)px`; update the padding to apply units via calc so the
unitless CSS variable is multiplied by px (e.g., use "1px
calc(var(--sb-appBorderRadius) * 1px)") — modify the styled span (DefaultExport)
to replace the current padding expression with a calc-based expression that
multiplies var(--sb-appBorderRadius) by 1px.
In @code/core/src/manager/components/sidebar/Menu.tsx:
- Around line 46-53: The styles in Menu.tsx use quoted CSS variable expressions
like 'var(--sb-background-app)' which makes them literal strings and breaks CSS;
update the style values for background, border and box-shadow in the affected
rule and the &:after rule to remove the surrounding quotes so the CSS custom
properties are used (e.g. use var(--sb-background-app) and
var(--sb-color-positive) directly and for box-shadow include the var(...) inside
the shadow string such as "0 0 0 2px var(--sb-background-app)").
In @code/core/src/manager/components/upgrade/UpgradeBlock.tsx:
- Around line 71-81: The ButtonTab styled component uses an invalid CSS value
'none' for borderBottomColor; update the borderBottomColor expression in
ButtonTab (the styled.button with prop active) to use 'transparent' when active
is false (e.g., borderBottomColor: active ? 'var(--sb-color-secondary)' :
'transparent') so the border is hidden correctly without an invalid value.
🧹 Nitpick comments (28)
code/core/src/manager/settings/Checklist/Checklist.stories.tsx (1)
48-50: LGTM!The migration from Emotion theme callback to CSS variable is clean and aligns with the PR objectives.
Nitpick: The template literal is unnecessary since there's no interpolation—a plain string would be slightly more idiomatic.
✏️ Optional: use plain string
const Container = styled.div({ - fontSize: `var(--sb-typography-size-s2)`, + fontSize: 'var(--sb-typography-size-s2)', });code/core/src/manager/settings/GuidePage.tsx (1)
23-37: Inconsistent quote style for CSS variable values.Line 29 uses template literals while line 30 uses single quotes. For consistency, consider using the same style throughout.
Suggested fix for consistency
'& h1': { fontSize: `var(--sb-typography-size-m3)`, - fontWeight: 'var(--sb-typography-weight-bold)', + fontWeight: `var(--sb-typography-weight-bold)`, margin: 0, },code/core/src/components/components/placeholder/placeholder.tsx (1)
11-16: Migration looks correct; consider verifying the calc() approach.The Message component successfully migrates to CSS variables. The
calc(var(--sb-typography-size-s2) - 1px)expression on Line 15 is functional but differs from similar components (e.g., Modal.styled.tsx uses plain CSS variables without calc()).If this -1px adjustment was in the original theme-based code, this preserves that behavior. Otherwise, consider whether the adjustment could be incorporated into the CSS variable definition for consistency.
🔍 Optional: Verify calc() consistency
#!/bin/bash # Description: Check how other components handle typography size variables. # Look for other calc() expressions or similar font-size adjustments. # Search for calc() with typography size variables rg -n "calc\(var\(--sb-typography-size" --type=tsx --type=ts -C2 # Search for direct typography size variable usage for comparison rg -n "fontSize.*var\(--sb-typography-size" --type=tsx --type=ts -C1code/core/src/manager/settings/About.tsx (1)
36-38: Unnecessary template literal on line 38.Lines 36-37 and the StyledLink use regular strings for CSS variable references, but line 38 uses a template literal without any interpolation. For consistency, use a regular string.
🔧 Suggested fix
color: theme.base === 'light' ? 'var(--sb-color-dark)' : 'var(--sb-color-lightest)', fontWeight: 'var(--sb-typography-weight-regular)', - fontSize: `var(--sb-typography-size-s2)`, + fontSize: 'var(--sb-typography-size-s2)',code/core/src/manager/components/error-boundary/ManagerErrorBoundary.tsx (1)
74-107: Incomplete migration:CollapseToggleandErrorMessagestill depend ontheme.These components are partially migrated—some properties use CSS variables while others still rely on
theme.basefor conditional logic:
- Line 88: Hover background uses
theme.base === 'light'conditional- Line 101: Background color uses
transparentize()withtheme.baseandtheme.color.negativeConsider completing the migration by introducing CSS variables for these cases (e.g.,
--sb-hover-background,--sb-error-background-subtle) that are defined differently for light/dark themes. Alternatively, if this hybrid approach is intentional, a brief comment explaining why would help future maintainers.code/core/src/manager/components/notifications/NotificationItem.tsx (1)
124-130: Partial migration is acceptable here.The
themedependency is retained becausetransparentizefrom polished requires an actual color value rather than a CSS variable string. For a future enhancement, consider using CSScolor-mix()(e.g.,color-mix(in srgb, var(--sb-color-inverseText) 75%, transparent)) or defining a pre-computed--sb-color-inverseText-75variable to complete the migration.code/core/src/components/components/typography/elements/Table.tsx (1)
5-17: Partial migration:themecallback retained for a single conditional.Line 17 still uses
theme.baseto choose between CSS variables, which keeps this component dependent on the theme object at runtime. Consider introducing a single semantic variable (e.g.,--sb-table-row-alt-bg) that's defined differently in light/dark theme contexts. This would allow removing the callback entirely:-export const Table = styled.table(withReset, withMargin, ({ theme }) => ({ +export const Table = styled.table(withReset, withMargin, { fontSize: 'var(--sb-typography-size-s2)', ... '& tr:nth-of-type(2n)': { - backgroundColor: theme.base === 'dark' ? 'var(--sb-color-darker)' : 'var(--sb-color-lighter)', + backgroundColor: 'var(--sb-table-row-alt-bg)', }, ... -})); +});If this is intentional for this PR scope, please disregard.
code/core/src/components/components/Form/Checkbox.tsx (1)
5-48: Inconsistent approach to styling: line 8 uses hardcoded colors instead of CSS variables.Line 7 uses
var(--sb-input-background)but line 8 still uses hardcoded HSL values withtheme.baseconditional, while line 23 in the same component already usesvar(--sb-input-border). This creates an inconsistent pattern within a single styled component.Line 26's approach (using
theme.baseto select between CSS variables) is consistent with common patterns in the codebase, but line 8 could be improved by migrating the hardcoded border colors to CSS variables or a similar conditional CSS variable approach.code/core/src/components/components/typography/DocumentWrapper.tsx (2)
69-71: Consider using a CSS variable for the absent link color.The hardcoded
#cc0000color could be migrated to a CSS variable (e.g.,--sb-color-negativeor similar) for consistency with the rest of the migration.
301-309: Consider unifying the conditional color into a single CSS variable.Line 308 uses a JS conditional based on
theme.baseto select between two CSS variables. This hybrid approach works but doesn't fully leverage CSS variables for automatic theme switching.A dedicated CSS variable (e.g.,
--sb-code-text-color) that resolves to the appropriate value per theme would eliminate the JS dependency.Suggested approach
'code, tt': { margin: '0 2px', padding: '0 5px', whiteSpace: 'nowrap', border: `1px solid var(--sb-color-mediumlight)`, backgroundColor: 'var(--sb-color-lighter)', borderRadius: 3, - color: theme.base === 'dark' ? 'var(--sb-color-darkest)' : 'var(--sb-color-dark)', + color: 'var(--sb-code-text-color)', },This would require adding
--sb-code-text-colorto theThemeVariablescomponent with appropriate light/dark values.Is this hybrid pattern intentional for this migration phase, or should all theme conditionals be eliminated in favor of pure CSS variable usage?
code/core/src/manager/components/sidebar/RefBlocks.tsx (2)
24-28: Template literal is unnecessary for simple string values.The backticks around the CSS variable value are not needed since there's no interpolation.
✨ Suggested simplification
const TextStyle = styled.div({ - fontSize: `var(--sb-typography-size-s2)`, + fontSize: 'var(--sb-typography-size-s2)', lineHeight: '20px', margin: 0, });
29-43: Same template literal simplification applies here.Consider using regular string quotes for consistency and simplicity.
✨ Suggested simplification
const Text = styled.div({ - fontSize: `var(--sb-typography-size-s2)`, + fontSize: 'var(--sb-typography-size-s2)', lineHeight: '20px', margin: 0, code: { - fontSize: `var(--sb-typography-size-s1)`, + fontSize: 'var(--sb-typography-size-s1)', }, ul: { paddingLeft: 20, marginTop: 8, marginBottom: 8, }, });code/core/src/component-testing/components/MatcherResult.tsx (1)
41-51: Consider migrating hardcoded hex colors to CSS variables for consistency.Lines 41 and 51 use hardcoded hex colors (
#D43900,#16B242) while the styled components now use CSS variables. For theming consistency, these inline styles could also use CSS variables.♻️ Suggested refactor
- <Node showObjectInspector value={value} style={{ color: '#D43900' }} /> + <Node showObjectInspector value={value} style={{ color: 'var(--sb-color-negative)' }} />- return <Node showObjectInspector value={value} style={{ color: '#16B242' }} />; + return <Node showObjectInspector value={value} style={{ color: 'var(--sb-color-positive)' }} />;code/core/src/manager/components/sidebar/StatusButton.tsx (1)
49-55: Partial migration creates inconsistency.The
colorproperty uses CSS variables whilebackgroundstill uses theme tokens withdarken/lighten. This is similar to the pattern in ToggleButton.tsx and creates a mixed approach within the same style block.code/core/src/actions/components/ActionLogger/style.tsx (1)
14-23: Partial migration leaves theme dependency.The
Countercomponent partially migrates to CSS variables (color, fontSize, fontWeight) but retainstheme.appBorderColorwithopacify()for backgroundColor. This is likely intentional sinceopacify()requires a resolved color value and cannot work with CSS variables.Consider whether the opacity effect could be achieved using a dedicated CSS variable (e.g.,
--sb-appBorderColor-faded) or CSScolor-mix()to complete the migration and remove the theme dependency entirely.code/core/src/manager/components/mobile/about/MobileAbout.tsx (1)
145-171: Consider extracting the repeated font size calculation.The expression
calc(var(--sb-typography-size-s2) - 1px)is duplicated inLinkLine,LinkLeft, andBottomText. Consider extracting this to a CSS variable or a shared constant for maintainability.♻️ Optional: Extract shared font size
You could define this once at the top level or as a shared variable:
const smallFontSize = 'calc(var(--sb-typography-size-s2) - 1px)'; const LinkLine = styled.a({ // ... fontSize: smallFontSize, // ... });Alternatively, if this pattern is used across multiple files, consider adding a dedicated CSS variable like
--sb-typography-size-s2-minus-1in the theme variables definition.code/core/src/theming/ThemeVariables.tsx (1)
6-22: Consider handlingnullvalues.The function filters out
undefinedand functions, butnullvalues would pass through and be stringified as"null"in the CSS output, resulting in invalid CSS like--sb-someKey: null.Suggested improvement
const getVariables = (theme: object, prefix = '--sb'): string[] => Object.entries(theme).reduce<string[]>((acc, [key, value]) => { - if (value === undefined || typeof value === 'function') { + if (value === undefined || value === null || typeof value === 'function') { return acc; }code/core/src/manager/components/sidebar/TestingWidget.tsx (1)
105-133: Incomplete theme migration in StatusButton.This component has a mixed approach: it uses CSS variables for some colors (lines 115-121, 129-131) but still relies on
theme.baseandtheme.color.*for the dark mode background calculation (lines 125-126). This inconsistency could cause maintenance issues.Consider defining CSS variables for the semi-transparent backgrounds (e.g.,
--sb-background-negative-subtle,--sb-background-warning-subtle) to complete the migration and remove thethemedependency entirely.code/core/src/manager/components/sidebar/ChecklistWidget.tsx (1)
108-137: Incomplete theme migration in ItemLabel.The component still destructures
theme(line 109) and usestheme.base === 'dark'(line 116) for conditional styling, while other values are migrated to CSS variables. This creates an inconsistent pattern.Consider creating a CSS variable that handles the light/dark distinction (e.g.,
--sb-color-positive-textthat resolves differently per theme), or use a CSS selector approach like[data-theme="dark"] &to eliminate the runtime theme dependency.Potential approach
-const ItemLabel = styled.span<{ isCompleted: boolean; isSkipped: boolean }>( - ({ theme, isCompleted, isSkipped }) => ({ +const ItemLabel = styled.span<{ isCompleted: boolean; isSkipped: boolean }>( + ({ isCompleted, isSkipped }) => ({ position: 'relative', margin: '0 -2px', padding: '0 2px', color: isSkipped ? 'var(--sb-color-mediumdark)' : isCompleted - ? theme.base === 'dark' - ? 'var(--sb-color-positive)' - : 'var(--sb-color-positiveText)' + ? 'var(--sb-color-positiveText)' // Ensure this CSS var handles light/dark : 'var(--sb-color-defaultText)', transition: 'color 500ms', }),code/core/src/manager/components/sidebar/Search.tsx (1)
129-144: Theme parameter retained for conditional logic.The
FocusKeycomponent retains thethemeparameter to conditionally apply different colors based ontheme.base === 'light'. This is appropriate since CSS variables alone cannot provide this conditional logic at the component level without additional CSS class toggling.Consider whether a CSS-based approach using a class or data attribute for theme mode (e.g.,
[data-theme="light"]) could eliminate this remaining theme dependency for full consistency with the migration. However, this is optional and the current approach works correctly.code/core/src/component-testing/components/InteractionsPanel.tsx (1)
58-67: Inconsistent theme migration in dark mode branch.The
CaughtExceptioncomponent still usestheme.color.negativedirectly in the dark mode branch rather than a CSS variable. This creates an inconsistency where the light mode usesvar(--sb-background-warning)but dark mode uses the theme object.Consider creating a CSS variable for the dark mode background color (e.g.,
var(--sb-background-warning-dark)) or using a single CSS variable that automatically adapts based on the theme mode.Potential approach
const CaughtException = styled.div(({ theme }) => ({ borderBottom: `1px solid var(--sb-appBorderColor)`, - backgroundColor: - theme.base === 'dark' - ? transparentize(0.93, theme.color.negative) - : 'var(--sb-background-warning)', + backgroundColor: 'var(--sb-background-warning)', padding: 15, fontSize: `calc(var(--sb-typography-size-s2) - 1px)`, lineHeight: '19px', }));This would require defining
--sb-background-warningto handle both light and dark themes appropriately in the theme variable definitions.code/core/src/components/components/Loader/Loader.tsx (1)
43-53: MigrateProgressTrackto use CSS variables for consistency withProgressBar.The
ProgressTrackcomponent usestransparentize(0.8, theme.color.secondary)while the adjacentProgressBarcomponent usesvar(--sb-color-secondary). To align with the CSS variable migration, consider updatingProgressTrackto use CSS variables instead of the theme object withpolished—for example, using CSScolor-mix()with an alpha channel or defining a--sb-color-secondary-transparentvariable.code/core/src/component-testing/components/Interaction.tsx (1)
125-130: Inconsistent mixing of theme and CSS variables.
ErrorExplainerusestheme.basefor conditionals but references CSS variables (--sb-color-negative,--sb-color-negativeText). This creates an unusual pattern where the component depends on theme but uses CSS variables for the actual values. Consider either fully migrating by using a CSS-based dark mode selector, or keeping theme values for consistency withErrorNameandErrorMessageabove.code/core/src/components/components/typography/elements/Code.tsx (1)
28-41: Incomplete migration:themecallback retained forboxShadow.The component still uses
({ theme })callback (line 28) to accesstheme.basefor the conditionalboxShadowon line 36. Consider introducing a CSS variable for the box-shadow or using a CSS custom property that changes based on the theme, to complete the migration and remove the theme dependency.If keeping the
theme.basecheck is intentional for now, this is acceptable as a transitional state, but it would be worth tracking for a follow-up.code/core/src/manager/components/sidebar/TreeNode.tsx (1)
9-38: Incomplete migration:theme.basestill used for conditional colors.The
TypeIconcomponent still relies ontheme.baseto conditionally select colors (lines 16 and 24). Additionally, line 24 uses a hardcoded color#ff8300instead of a CSS variable, which is inconsistent with the migration pattern.Consider:
- Creating theme-aware CSS variables (e.g.,
--sb-color-document) that automatically switch based on the active theme, or- Tracking this as a follow-up task if the theme-aware variable infrastructure isn't ready yet.
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx (1)
109-114: Pre retains theme dependency for layoutMargin.The
Precomponent still usestheme.layoutMargindirectly. This appears to be intentionally left as a theme reference, possibly because it's used conditionally with padding. For consistency, consider migrating tovar(--sb-layoutMargin)similar to line 123.Suggested refactor for consistency
-const Pre = styled.pre<PreProps>(({ theme, padded }) => ({ +const Pre = styled.pre<PreProps>(({ padded }) => ({ display: 'flex', justifyContent: 'flex-start', margin: 0, - padding: padded ? theme.layoutMargin : 0, + padding: padded ? 'var(--sb-layoutMargin)' : 0, }));code/core/src/components/components/typography/link/link.tsx (1)
102-133: Secondary/tertiary variants still use theme tokens.This block continues to reference
theme.textMutedColor,theme.color.secondary, andtheme.color.darkdirectly. For consistency with the migration pattern, consider updating these to use the corresponding CSS variables.code/core/src/components/components/Button/Button.tsx (1)
241-259: Inconsistent CSS variable usage in color logic.Some branches use CSS variables while others still reference theme tokens directly. Consider aligning for consistency where polished functions aren't needed:
- Line 243:
theme.color.lightest→ could usevar(--sb-color-lightest)- Line 247:
theme.input.color→ could usevar(--sb-input-color)(already done on line 259)♻️ Suggested alignment
color: (() => { if (variant === 'solid') { - return theme.color.lightest; + return 'var(--sb-color-lightest)'; } if (variant === 'outline') { - return theme.input.color; + return 'var(--sb-input-color)'; }
|
What is the goal here? It's important we move to CSS custom props and I can't wait to do that. I am not okay with doing it without discussion. Custom props will effectively be one method users can use to reliably modify our theme. In other words, it has similar constraints as an API. I am not okay doing this until we align on tokens. Please feel free to jump in the discussion here or jump on a call with both myself and @Sidnioulz (any else is welcome). I have done some spiking on a branch to try out various ways of organizing or implementing these. Check out what I have done here. It is far from complete. All of the work I have been working on around tokens is in service of:
I am not convinced we are far enough along with tokens yet to try to make this PR happen. I encourage others to weigh in. @JReinhold |
|
@MichaelArestad Don't worry, this is just a stepping stone towards that goal, it's a spike. I have no intention of shipping this as-is. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx (1)
120-126: Inconsistent theme variable usage:Prestill usestheme.layoutMarginwhileCodeuses the CSS variable.
Codenow usesvar(--sb-layoutMargin)forpaddingRight(line 123), butPreon line 113 still accessestheme.layoutMargin. This creates an inconsistency within the same file—if the CSS variable value diverges from the theme value, the components will behave differently.Consider aligning
Preto also usevar(--sb-layoutMargin):Suggested change for Pre
-const Pre = styled.pre<PreProps>(({ theme, padded }) => ({ +const Pre = styled.pre<PreProps>(({ padded }) => ({ display: 'flex', justifyContent: 'flex-start', margin: 0, - padding: padded ? theme.layoutMargin : 0, + padding: padded ? 'var(--sb-layoutMargin)' : 0, }));code/core/src/manager/components/sidebar/FileList.tsx (3)
36-36: Inconsistent migration: mixingtheme.baseconditionals with CSS variables.This pattern appears throughout the file (lines 36, 66, 82-83, 88-90, 165-167, etc.). Using
theme.baseto choose between a hardcoded value and a CSS variable partially defeats the purpose of CSS variables for theming.Consider defining separate CSS variables for dark/light modes (e.g.,
--sb-color-hover-bg) that automatically resolve based on the theme context, eliminating the need for runtime conditionals.
81-95: Hardcoded hex colors remain un-migrated.Several hardcoded colors (
#00000011,#00000022,#00000033, and#F9ECECat line 158) are used for error states. For a complete CSS variables migration, these should also be converted to semantic variables like--sb-color-error-bgor similar.
207-213: Inconsistent: hardcoded#000for light mode.Line 212 uses a CSS variable for dark mode but a hardcoded
#000for light mode. For consistency, consider usingvar(--sb-color-darkest)or a similar semantic variable.- color: theme.base === 'dark' ? 'var(--sb-color-lightest)' : '#000', + color: theme.base === 'dark' ? 'var(--sb-color-lightest)' : 'var(--sb-color-darkest)',
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
code/.storybook/preview.tsxcode/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsxcode/core/src/manager/components/sidebar/FileList.tsxcode/core/src/manager/components/upgrade/UpgradeBlock.tsxcode/core/src/theming/ThemeVariables.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- code/core/src/manager/components/upgrade/UpgradeBlock.tsx
- code/core/src/theming/ThemeVariables.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with
yarn prettier --write <file>
Files:
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsxcode/core/src/manager/components/sidebar/FileList.tsx
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run ESLint checks using
yarn lint:js:cmd <file>or the full commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsxcode/core/src/manager/components/sidebar/FileList.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsxcode/core/src/manager/components/sidebar/FileList.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsxcode/core/src/manager/components/sidebar/FileList.tsx
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsxcode/core/src/manager/components/sidebar/FileList.tsx
🧠 Learnings (4)
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Applied to files:
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsxcode/core/src/manager/components/sidebar/FileList.tsx
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsxcode/.storybook/preview.tsxcode/core/src/manager/components/sidebar/FileList.tsx
📚 Learning: 2025-10-01T15:24:01.060Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32594
File: code/core/src/components/components/Popover/WithPopover.tsx:7-9
Timestamp: 2025-10-01T15:24:01.060Z
Learning: In the Storybook repository, "react-aria-components/patched-dist/*" (e.g., "react-aria-components/patched-dist/Dialog", "react-aria-components/patched-dist/Popover", "react-aria-components/patched-dist/Tooltip") are valid import paths created by a patch applied to the react-aria-components package. These imports should not be flagged as broken or invalid until a maintainer explicitly states they are no longer acceptable.
Applied to files:
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.
Applied to files:
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx
🧬 Code graph analysis (1)
code/.storybook/preview.tsx (1)
code/core/src/theming/ThemeVariables.tsx (1)
ThemeVariables(24-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
- GitHub Check: nx
🔇 Additional comments (6)
code/core/src/components/components/syntaxhighlighter/syntaxhighlighter.tsx (2)
68-91: CSS variable migration looks correct for Wrapper.The migration from theme tokens to CSS variables for
color,border,borderRadius, andbackgroundis consistent with the PR objectives.
98-103:Scrollerstill relies onthemefor syntax highlighting styles.Line 102 passes the theme to
themedSyntax(theme), which generates styles fromtheme.code. If the goal is to fully migrate away from Emotion theme variables, this usage should also be converted to CSS variables. Otherwise,Scrollerwill continue to depend on the theme object while other components in this file do not.Is the partial migration intentional for this PR, or should
themedSyntaxalso be refactored to use CSS variables for consistency?code/.storybook/preview.tsx (2)
296-314: LGTM!Default case correctly uses
ThemeVariableswithout arootSelector, applying CSS variables at:rootlevel for global scope when rendering a single theme.
247-267: Remove theconsole.logstatement fromThemeVariables.tsxline 31.The
console.log(variables)debug statement should be removed before merging, as it violates coding guidelines against leaving debug logs in production code.The ThemeVariables integration for side-by-side rendering is correct—the scoped
rootSelectorapproach properly isolates CSS variables to each theme block via matchingidattributes, ensuring light and dark themes can coexist without conflicts.⛔ Skipped due to learnings
Learnt from: CR Repo: storybookjs/storybook PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-12-22T22:03:40.123Z Learning: Applies to **/*.{ts,tsx,js,jsx} : Do not use `console.log`, `console.warn`, or `console.error` directly unless in isolated files where importing loggers would significantly increase bundle sizeLearnt from: Sidnioulz Repo: storybookjs/storybook PR: 32458 File: code/core/src/components/components/Select/Select.tsx:200-204 Timestamp: 2025-11-05T09:38:47.712Z Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.Learnt from: CR Repo: storybookjs/storybook PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-12-22T22:03:40.123Z Learning: Applies to code/{renderers}/**/*.{ts,tsx,js,jsx} : Use `logger` from `storybook/internal/client-logger` for client-side logging in browser codecode/core/src/manager/components/sidebar/FileList.tsx (2)
112-136: Good migration pattern.
FileListIconWrapperandFileListItemPathare cleanly migrated—the theme parameter is removed and CSS variables are used consistently. This is the pattern to follow for the remaining components.
215-218: LGTM!Clean migration with theme parameter removed and CSS variable used for the muted text color.
|
@ghengeveld oh! Phew! I would love to chat about it. I have a bunch of things I ran into when messing with that linked branch above in terms of ergonomics and complexity. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @code/core/src/components/components/Popover/PopoverProvider.tsx:
- Line 2: Remove the unused React hook import by deleting "useMemo" from the
import list in the top-level React import (the line that currently reads "import
React, { useCallback, useMemo, useRef, useState } from 'react';"); update it to
only include the hooks actually used (useCallback, useRef, useState) and run the
linter to confirm the unused-import warning is resolved.
🧹 Nitpick comments (3)
code/core/src/components/components/Form/Checkbox.tsx (3)
8-8: Border color not migrated to CSS variable.Line 8 still uses
theme.baseconditional logic while surrounding properties have been migrated to CSS variables. Consider introducing a CSS variable like--sb-input-border-colorfor consistency.
25-27: Mixed approach:theme.basecheck with CSS variables.This conditional defeats the purpose of CSS variables for theming—CSS variables should encapsulate theme-specific values so JS doesn't need to branch on
theme.base. Consider defining a single variable (e.g.,--sb-color-disabled-checked-bg) that theThemeVariablescomponent sets differently per theme.♻️ Suggested approach
'&:disabled:checked, &:disabled:indeterminate': { - backgroundColor: theme.base === 'dark' ? 'var(--sb-color-dark)' : 'var(--sb-color-mediumdark)', + backgroundColor: 'var(--sb-color-disabled-checked-bg)', },Then define
--sb-color-disabled-checked-bginThemeVariablesto resolve to--sb-color-darkor--sb-color-mediumdarkbased on the theme.
28-31: Inconsistent use ofcolor.secondaryvs CSS variable.Line 30 uses the imported
color.secondarytoken while line 45 usesvar(--sb-color-secondary)for the same semantic color. For a consistent migration, both should use the CSS variable. If there's a dark-theme limitation withtheme.color.secondary, the CSS variable approach (which derives from the runtime theme) should actually resolve it.♻️ Proposed fix
'&:checked, &:indeterminate': { border: 'none', - backgroundColor: color.secondary, // TODO Can't use theme.color.secondary on dark theme + backgroundColor: 'var(--sb-color-secondary)', },
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
code/.storybook/preview.tsxcode/chromatic.config.jsoncode/core/src/components/components/Form/Checkbox.tsxcode/core/src/components/components/Popover/PopoverProvider.tsxcode/core/src/components/components/Tabs/Tabs.hooks.tsxcode/core/src/manager/components/sidebar/Menu.tsxcode/core/src/theming/ThemeVariables.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- code/core/src/theming/ThemeVariables.tsx
- code/core/src/components/components/Tabs/Tabs.hooks.tsx
- code/core/src/manager/components/sidebar/Menu.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with
yarn prettier --write <file>
Files:
code/core/src/components/components/Popover/PopoverProvider.tsxcode/chromatic.config.jsoncode/core/src/components/components/Form/Checkbox.tsx
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run ESLint checks using
yarn lint:js:cmd <file>or the full commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/core/src/components/components/Popover/PopoverProvider.tsxcode/chromatic.config.jsoncode/core/src/components/components/Form/Checkbox.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/components/components/Popover/PopoverProvider.tsxcode/core/src/components/components/Form/Checkbox.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/components/components/Popover/PopoverProvider.tsxcode/core/src/components/components/Form/Checkbox.tsx
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/core/src/components/components/Popover/PopoverProvider.tsxcode/core/src/components/components/Form/Checkbox.tsx
🧠 Learnings (5)
📚 Learning: 2025-09-24T09:39:39.233Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32507
File: code/core/src/manager/globals/globals-module-info.ts:25-33
Timestamp: 2025-09-24T09:39:39.233Z
Learning: In Storybook, storybook/actions/decorator is a preview-only entrypoint and should not be included in manager globals configuration. The duplicatedKeys array in code/core/src/manager/globals/globals-module-info.ts is specifically for manager-side externalization, not preview entrypoints.
Applied to files:
code/.storybook/preview.tsx
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/.storybook/preview.tsxcode/core/src/components/components/Popover/PopoverProvider.tsx
📚 Learning: 2025-11-25T11:09:33.798Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 33140
File: code/core/src/manager/components/sidebar/TagsFilter.tsx:247-259
Timestamp: 2025-11-25T11:09:33.798Z
Learning: In the storybookjs/storybook repository, PopoverProvider creates popovers with a dialog role, so using aria-haspopup="dialog" on buttons that trigger PopoverProvider is semantically correct.
Applied to files:
code/core/src/components/components/Popover/PopoverProvider.tsx
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.
Applied to files:
code/core/src/components/components/Popover/PopoverProvider.tsx
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Applied to files:
code/core/src/components/components/Popover/PopoverProvider.tsx
🧬 Code graph analysis (2)
code/.storybook/preview.tsx (4)
code/core/src/theming/ThemeVariables.tsx (1)
ThemeVariables(24-41)code/core/src/theming/index.ts (1)
ThemeProvider(17-17)code/core/src/docs-tools/argTypes/convert/typescript/convert.ts (1)
convert(32-71)code/core/src/theming/create.ts (1)
themes(20-23)
code/core/src/components/components/Form/Checkbox.tsx (1)
code/core/src/theming/base.ts (1)
color(1-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
- GitHub Check: nx
🔇 Additional comments (8)
code/core/src/components/components/Form/Checkbox.tsx (1)
44-47: LGTM!Focus outline correctly uses the CSS variable
var(--sb-color-secondary).code/.storybook/preview.tsx (4)
22-31: LGTM - Import addition is correct.
ThemeVariablesis properly imported alongside other theming utilities.
240-270: Side-by-side theme implementation is well-structured.The pattern correctly:
- Sets root-level CSS variables to light theme as a fallback (line 245)
- Scopes theme-specific CSS variables to each container via matching
idandrootSelectorattributesThis ensures components using CSS variables will pick up the correct theme values within each panel.
272-294: LGTM - Stacked theme mode follows the same consistent pattern.The implementation mirrors the side-by-side approach with properly scoped CSS variables for each theme stack.
296-317: LGTM - Default theme mode correctly injects CSS variables.
ThemeVariablesusesuseTheme()internally, so it will correctly emit CSS variables for whatever theme is provided by the parentThemeProvider.code/chromatic.config.json (1)
3-7: LGTM - Chromatic configuration additions are correct.The
buildScriptName("storybook:ui:build"),storybookBaseDir("code"), and other fields are valid Chromatic configuration options aligned with the monorepo structure. The referenced build script exists in code/package.json.code/core/src/components/components/Popover/PopoverProvider.tsx (2)
94-109: Structure looks good pendingboundaryElementfix.The
Pressablewrapper correctly forwards the ref for DOM access, and thePopoverUpstreamconfiguration with placement, offset, and boundary handling aligns with the theming integration goal. Theoutline: 'none'is acceptable since react-aria's dialog/popover components manage focus internally.
69-72: No changes needed toboundaryElementlogic.The component correctly handles the ref timing. While
elementRef.currentisnullduring the initial render, falling back todocument.bodyis the intended behavior and works correctly—the test storyAlwaysOpenverifies thatvisible={true}renders the popover as expected. The fallback is safe and sensible; no refactoring is required.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@code/core/src/components/components/Popover/PopoverProvider.tsx`:
- Around line 69-72: The boundaryElement is computed too early because
elementRef.current is null on first render, so replace the inline computation
with a stateful value: initialize boundaryElement state to document.body, then
in a useLayoutEffect read elementRef.current and setBoundaryElement to
elementRef.current?.closest('[id^="sb-theme-"]') || document.body; also
re-run/update when the ref changes or when running in Storybook (check
globalThis.__STORYBOOK_PREVIEW__) so that opening on defaultVisible/visible uses
the correct Storybook theme root.
🧹 Nitpick comments (5)
code/core/src/manager/components/sidebar/TestingWidget.tsx (1)
106-134: Inconsistent migration in dark theme backgrounds.The light theme is fully migrated to CSS variables, but the dark theme background (lines 126-128) still relies on
theme.color.negativeandtheme.color.warningto append22opacity. This keeps thethemedependency on this component.Consider defining dedicated CSS variables with the opacity baked in (e.g.,
--sb-background-negative-muted) to complete the migration and remove the theme dependency entirely. This would also make the dark/light theme logic more symmetric.code/core/src/manager/components/sidebar/Sidebar.tsx (1)
56-61: CSS variable naming inconsistency.The changes are functionally correct, but the CSS variable naming conventions differ between components:
Containeruses kebab-case:--sb-background-content,--sb-background-appCreateNewStoryButtonuses camelCase:--sb-textMutedColor,--sb-appBorderRadiusThis likely mirrors the original
themeobject structure (e.g.,theme.background.contentvstheme.textMutedColor), but since this PR establishes CSS variables as a quasi-public API surface (as noted in PR comments), consider standardizing on one naming convention for consistency. Kebab-case is more idiomatic for CSS custom properties.code/core/src/manager/settings/shortcuts.tsx (3)
18-24: Inconsistent string syntax and potential TypeScript type issue.
Line 20 uses a template literal for
fontSizewhile line 21 uses a plain string forfontWeight. For consistency, prefer plain strings when no interpolation is needed.
fontWeight: 'var(--sb-typography-weight-bold)'may cause TypeScript errors in strict mode becauseCSSProperties['fontWeight']expects specific values (e.g.,'bold','normal',number), not arbitrary strings.Suggested fix
const Header = styled.header({ marginBottom: 20, - fontSize: `var(--sb-typography-size-m3)`, - fontWeight: 'var(--sb-typography-weight-bold)', + fontSize: 'var(--sb-typography-size-m3)', + fontWeight: 'var(--sb-typography-weight-bold)' as React.CSSProperties['fontWeight'], alignItems: 'center', display: 'flex', });
27-29: SamefontWeighttype consideration applies here.See the comment on
Headerabove regarding the potential TypeScript strict mode issue with CSS variable strings forfontWeight.
110-115: Unnecessary template literal.For consistency with other CSS variable usages in this file (e.g., line 21, 28, 45, 95), prefer a plain string since no interpolation is needed.
Suggested fix
const Container = styled.div({ - fontSize: `var(--sb-typography-size-s2)`, + fontSize: 'var(--sb-typography-size-s2)', padding: `3rem 20px`, maxWidth: 600, margin: '0 auto', });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
code/core/src/components/components/Card/Card.tsxcode/core/src/components/components/Popover/PopoverProvider.tsxcode/core/src/manager/components/layout/Layout.tsxcode/core/src/manager/components/mobile/navigation/MobileNavigation.tsxcode/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/preview/Toolbar.tsxcode/core/src/manager/components/preview/utils/components.tscode/core/src/manager/components/sidebar/Heading.tsxcode/core/src/manager/components/sidebar/Search.tsxcode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/components/sidebar/Tree.tsxcode/core/src/manager/settings/shortcuts.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- code/core/src/manager/components/mobile/navigation/MobileNavigation.tsx
- code/core/src/manager/components/sidebar/Search.tsx
- code/core/src/components/components/Card/Card.tsx
- code/core/src/manager/components/sidebar/Heading.tsx
- code/core/src/manager/components/preview/Toolbar.tsx
- code/core/src/manager/components/preview/utils/components.ts
- code/core/src/manager/components/sidebar/Tree.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with
yarn prettier --write <file>
Files:
code/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/components/components/Popover/PopoverProvider.tsxcode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/layout/Layout.tsxcode/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/settings/shortcuts.tsx
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run ESLint checks using
yarn lint:js:cmd <file>or the full commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
code/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/components/components/Popover/PopoverProvider.tsxcode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/layout/Layout.tsxcode/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/settings/shortcuts.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
code/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/components/components/Popover/PopoverProvider.tsxcode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/layout/Layout.tsxcode/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/settings/shortcuts.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/components/components/Popover/PopoverProvider.tsxcode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/layout/Layout.tsxcode/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/settings/shortcuts.tsx
code/{core,lib,addons,builders,frameworks,presets}/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
loggerfromstorybook/internal/node-loggerfor server-side logging in Node.js code
Files:
code/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/components/components/Popover/PopoverProvider.tsxcode/core/src/manager/components/sidebar/Sidebar.tsxcode/core/src/manager/components/layout/Layout.tsxcode/core/src/manager/components/sidebar/TestingWidget.tsxcode/core/src/manager/settings/shortcuts.tsx
🧠 Learnings (6)
📓 Common learnings
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
📚 Learning: 2025-10-03T07:55:42.639Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/manager/components/preview/Toolbar.tsx:102-105
Timestamp: 2025-10-03T07:55:42.639Z
Learning: In code/core/src/manager/components/preview/Toolbar.tsx, we intentionally do not add aria-label/aria-labelledby to StyledToolbar (AbstractToolbar) because the enclosing section is already labeled via an sr-only heading and the toolbar is the only content. Revisit only if real user testing indicates a need.
Applied to files:
code/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/manager/components/sidebar/TestingWidget.tsx
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Applied to files:
code/core/src/manager/components/preview/FramesRenderer.tsxcode/core/src/components/components/Popover/PopoverProvider.tsxcode/core/src/manager/components/sidebar/Sidebar.tsx
📚 Learning: 2025-11-25T11:09:33.798Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 33140
File: code/core/src/manager/components/sidebar/TagsFilter.tsx:247-259
Timestamp: 2025-11-25T11:09:33.798Z
Learning: In the storybookjs/storybook repository, PopoverProvider creates popovers with a dialog role, so using aria-haspopup="dialog" on buttons that trigger PopoverProvider is semantically correct.
Applied to files:
code/core/src/components/components/Popover/PopoverProvider.tsx
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.
Applied to files:
code/core/src/components/components/Popover/PopoverProvider.tsxcode/core/src/manager/components/sidebar/TestingWidget.tsx
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/core/src/components/components/Popover/PopoverProvider.tsxcode/core/src/manager/components/sidebar/Sidebar.tsx
🧬 Code graph analysis (1)
code/core/src/manager/components/sidebar/Sidebar.tsx (1)
code/core/src/components/components/Modal/Modal.styled.tsx (1)
Container(96-159)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: normal
- GitHub Check: nx
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (11)
code/core/src/manager/components/preview/FramesRenderer.tsx (1)
22-37: LGTM — clean migration to CSS variable.The removal of the
themedependency and switch tovar(--sb-typography-size-s1)aligns with the PR's goal of migrating to CSS custom properties.One minor consideration: if the CSS variable isn't defined (e.g., during initial render before
ThemeVariablesinjects styles), there's no fallback. You could optionally add one:fontSize: `var(--sb-typography-size-s1, 12px)`This is a stylistic choice and may not be necessary if
ThemeVariablesis guaranteed to be present before this component renders.code/core/src/manager/components/layout/Layout.tsx (2)
242-279: CSS variable migration looks consistent across container components.The migration from theme-based styling to CSS variables (
--sb-appBg,--sb-appContentBg,--sb-appBorderColor) is applied uniformly acrossSidebarContainer,ContentContainer,PagesContainer, andPanelContainer. The conditional border logic inPanelContaineris correctly preserved.Consider adding fallback values for robustness (e.g.,
var(--sb-appBg, transparent)) to handle cases where the CSS variables might not be defined, though this may be unnecessary ifThemeVariablesis guaranteed to be rendered in the component tree.
281-297: LGTM!The
Dragcomponent correctly uses Emotion's multi-argument styled pattern: a static styles object followed by a dynamic styles function. The--sb-color-secondaryvariable for the drag indicator is appropriately placed.code/core/src/manager/components/sidebar/TestingWidget.tsx (2)
39-42: LGTM!Clean migration to CSS variables for the border color.
136-142: LGTM!Clean migration to CSS variables for the border separator.
code/core/src/manager/components/sidebar/Sidebar.tsx (1)
31-47: LGTM!The migration from theme-based styling to CSS variables is clean. The syntax change from a function wrapper to a plain object is correct since
themeis no longer accessed. The CSS variable naming (--sb-background-content,--sb-background-app) follows the consistent--sb-prefix pattern used elsewhere in this PR (e.g.,Modal.styled.tsx).code/core/src/manager/settings/shortcuts.tsx (3)
43-48: LGTM!The migration to CSS variable for
borderToplooks correct. Template literal is appropriate here for the composite value.Minor observation: the variable naming uses
--sb-appBorderColor(camelCase) while other variables use kebab-case (e.g.,--sb-typography-size-m3). This appears intentional to mirror existing theme property names, but worth ensuring consistency across the broader PR.
91-108: LGTM!The migration correctly removes the
themedependency and uses the CSS variable--sb-color-positivefor the success icon color. The localFadekeyframes animation is preserved appropriately.
66-84: Incomplete migration:TextInputstill uses theme.This component still references
theme.animation.jiggle(line 72) while other components in this file have been migrated to CSS variables. This appears inconsistent with the PR's migration pattern.If intentional (perhaps animation keyframes require different handling), consider adding a comment explaining why. Otherwise, this should be migrated to a CSS variable like
var(--sb-animation-jiggle)for consistency.code/core/src/components/components/Popover/PopoverProvider.tsx (2)
95-109: LGTM!The
boundaryElementprop integration andoutline: 'none'styling align with the CSS variables migration. The outline removal on the container is appropriate since react-aria manages focus within the popover separately.
94-94: VerifyPressableref forwarding works with theboundaryElementcomputation.
@react-aria/interactionsPressablewraps children and should forward therefprop to its underlying DOM element. Confirm thatelementRef.current?.closest('[id^="sb-theme-"]')will resolve correctly, as it depends onrefbeing properly forwarded.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| const elementRef = useRef<HTMLDivElement>(null); | ||
| const boundaryElement = | ||
| (globalThis.__STORYBOOK_PREVIEW__ && elementRef.current?.closest('[id^="sb-theme-"]')) || | ||
| document.body; |
There was a problem hiding this comment.
Edge case: boundaryElement computed before ref is populated on initial mount.
When defaultVisible or visible is true, the popover opens on first render while elementRef.current is still null, causing boundaryElement to fall back to document.body instead of the Storybook theme root. Since this is a spike PR, this may be acceptable, but for a production implementation, consider using useState with useLayoutEffect to update boundaryElement after mount.
🤖 Prompt for AI Agents
In `@code/core/src/components/components/Popover/PopoverProvider.tsx` around lines
69 - 72, The boundaryElement is computed too early because elementRef.current is
null on first render, so replace the inline computation with a stateful value:
initialize boundaryElement state to document.body, then in a useLayoutEffect
read elementRef.current and setBoundaryElement to
elementRef.current?.closest('[id^="sb-theme-"]') || document.body; also
re-run/update when the ref changes or when running in Storybook (check
globalThis.__STORYBOOK_PREVIEW__) so that opening on defaultVisible/visible uses
the correct Storybook theme root.
|
Closing due to inactivity and the number of merge conflicts. Please feel free to open a new PR after the conflicts and comments are resolved. Thanks! |
3 participants
Closes #
What I did
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 PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.