fix: Update Canvas Provider theming - #4060
Merged
alanbsmith merged 27 commits intoJul 31, 2026
Merged
Conversation
📝 WalkthroughWalkthroughCanvas Kit adds numerical and semantic Sana Canvas theming, scoped CSS-variable handling, popup style inheritance, updated Storybook examples, v16 documentation, and documentation build support. ChangesSana Canvas theming
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Workday/canvas-kit
 |
||||||||||||||||||||||||||||||||||||||||
| Project |
Workday/canvas-kit
|
| Branch Review |
mc-theming-sana-canvas
|
| Run status |
|
| Run duration | 02m 33s |
| Commit |
|
| Committer | Manuel Carrera |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
0
|
 |
0
|
 |
17
|
 |
0
|
 |
812
|
| View all changes introduced in this branch ↗︎ | |
UI Coverage
19.67%
|
|
|---|---|
|
|
1521
|
|
|
370
|
Accessibility
99.47%
|
|
|---|---|
|
|
5 critical
5 serious
0 moderate
2 minor
|
|
|
71
|
…era4/canvas-kit into mc-theming-sana-canvas
When data-theme="sana-canvas" is set globally, teams don't need to pass sanaCanvasProviderTheme to CanvasProvider since CSS variables naturally cascade to popup containers. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Popups naturally inherit CSS variables from [data-theme="sana-canvas"], making the theme prop unnecessary for most use cases.
Warns developers when they pass sanaCanvasProviderTheme to CanvasProvider but already have data-theme="sana-canvas" set globally, since CSS variables naturally cascade to popups in this case. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Added .mockImplementation(() => {}) to all console.warn spies to prevent
actual warnings from being printed to stderr during test runs. This improves
test hygiene and provides cleaner test output.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates CanvasProvider scoped theming to support a v16/Sana-aligned numerical brand theme shape, adds an explicit theming scope concept ('brand' | 'full'), and fixes popup/portal theming by forwarding the resolved brand CSS variables to popup containers without relying on Emotion’s legacy theme context.
Changes:
- Introduces numerical
brandtheming types/scope resolution and implements brand-scope vs full-scope CSS variable writing (including Sana theme presets). - Reworks popup theming (
usePopupStack) to consume brand CSS-variable style maps fromCanvasProvidervia a new context to ensure portaled popups inherit scoped brand variables. - Updates Storybook utilities and documentation/upgrade guides to reflect Sana setup (
data-theme="sana-canvas", sana CSS import ordering, and new scoped theming API), plus bumps@workday/canvas-tokens-webto4.4.0.
Reviewed changes
Copilot reviewed 35 out of 36 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Locks @workday/canvas-tokens-web to 4.4.0. |
| utils/storybook/index.ts | Re-exports new Storybook theming helpers/presets. |
| utils/storybook/customThemes.ts | Adds numerical theme presets and a helper to normalize themes for CanvasProvider. |
| utils/storybook/CanvasProviderDecorator.tsx | Updates Storybook decorator to stop forcing a default theme wrapper and only pass a theme when provided. |
| README.md | Updates root setup guidance to include Sana CSS vars and data-theme="sana-canvas". |
| package.json | Bumps @workday/canvas-tokens-web dependency to 4.4.0. |
| modules/styling/package.json | Bumps @workday/canvas-tokens-web to 4.4.0. |
| modules/styling-transform/spec/utils/handleColorSpace.spec.ts | Adds tests for handleColorSpace transform behavior. |
| modules/styling-transform/package.json | Bumps @workday/canvas-tokens-web to 4.4.0. |
| modules/react/text-input/stories/visualTesting.stories.tsx | Updates visual testing story to pass theme via StaticStates and normalize theme shape. |
| modules/react/text-area/stories/visualTesting.stories.tsx | Updates visual testing story to use numerical theme and normalized provider theme. |
| modules/react/testing/lib/StaticStates.tsx | Updates StaticStates to accept CanvasProviderTheme and support numerical themes. |
| modules/react/popup/spec/usePopupStack.spec.tsx | Adds coverage ensuring popup containers receive forwarded CSS variables across theme shapes. |
| modules/react/popup/lib/hooks/usePopupStack.ts | Switches popup theming to consume brand-style context instead of Emotion theme context. |
| modules/react/package.json | Bumps @workday/canvas-tokens-web to 4.4.0. |
| modules/react/common/stories/theming/ThemeComparison.stories.tsx | Adds a Storybook comparison fixture for global vs scoped Sana/brand themes. |
| modules/react/common/stories/theming/examples/BrandingFixture.tsx | Adds a visual fixture exercising multiple component consumers of brand tokens. |
| modules/react/common/stories/mdx/Theming.mdx | Rewrites theming docs for v16/Sana: CSS import order, data-theme, scoped numerical brand theme API. |
| modules/react/common/stories/mdx/examples/ThemingBrandScope.tsx | Adds an MDX example demonstrating brand-scope behavior. |
| modules/react/common/spec/useCanvasThemeToCssVars.spec.tsx | Adds tests for brand-scope CSS var writing behavior. |
| modules/react/common/spec/theming-types.spec.ts | Adds tests for numerical theme detection and scope resolution. |
| modules/react/common/spec/sanaTheme.spec.ts | Adds tests for Sana theme presets referencing CSS vars and producing CSS-variable output. |
| modules/react/common/spec/brandScope.spec.ts | Adds tests for brand-scope bundle writers and numerical theme writing behavior. |
| modules/react/common/lib/theming/types.ts | Adds numerical theme types (CanvasNumericalBrandTheme, CanvasProviderTheme), themeScope, and scope resolution. |
| modules/react/common/lib/theming/sanaTheme.ts | Introduces Sana theme presets for popup parity and scoped usage. |
| modules/react/common/lib/theming/README.md | Updates theming README for v16 and documents Sana/scoped theming. |
| modules/react/common/lib/theming/index.ts | Exports Sana presets from theming entry point. |
| modules/react/common/lib/theming/brandScope.ts | Adds brand-scope/full-scope writers and shortcut bundles for primary/error/caution/etc. |
| modules/react/common/lib/CanvasProvider.tsx | Reworks CanvasProvider to support numerical themes, explicit scope, and provide brand style via context for popups. |
| modules/preview-react/package.json | Bumps @workday/canvas-tokens-web to 4.4.0. |
| modules/labs-react/package.json | Bumps @workday/canvas-tokens-web to 4.4.0. |
| modules/docs/package.json | Bumps @workday/canvas-tokens-web to 4.4.0. |
| modules/docs/mdx/16.0-UPGRADE-GUIDE.mdx | Adds v16 Sana theme opt-in and scoped theming guidance, including the legacy scope change. |
| modules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.md | Mirrors upgrade guide updates for LLM docs. |
| modules/docs/llm/theming.md | Mirrors theming docs updates for LLM docs. |
| .storybook/preview.js | Clarifies Sana import ordering and cascade rationale; ensures Sana vars imported last. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Clarifies that the theme prop is not needed when data-theme="sana-canvas" is set globally, as popups inherit CSS variables naturally.
Documents that sanaCanvasProviderTheme is no longer required when using global Sana CSS, simplifying the setup for most teams. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
Adds missing 'Default Scope Change for Legacy Themes' section for consistency between MDX and markdown versions.
Shows that popups work correctly without sanaCanvasProviderTheme when global Sana CSS is active. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
mannycarrera4
marked this pull request as ready for review
July 31, 2026 18:08
sheelah
reviewed
Jul 31, 2026
sheelah
left a comment
sheelah
left a comment
Contributor
There was a problem hiding this comment.
Code review — 2 issues found in brandScope.ts
sheelah
reviewed
Jul 31, 2026
sheelah
reviewed
Jul 31, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (16)
modules/react/common/stories/mdx/examples/SimplifiedSanaSetup.tsx (1)
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the export name with the file name.
The file is
SimplifiedSanaSetup.tsxand the export isSimplifiedSetup. Rename the export toSimplifiedSanaSetup, and update the MDX import.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/react/common/stories/mdx/examples/SimplifiedSanaSetup.tsx` at line 8, Rename the exported component in SimplifiedSanaSetup.tsx from SimplifiedSetup to SimplifiedSanaSetup, and update the corresponding MDX import and references to use the new name.modules/react/common/stories/theming/ThemeComparison.stories.tsx (1)
41-41: 📐 Maintainability & Code Quality | 🔵 TrivialConsider the Chromatic snapshot cost.
This story renders four full
BrandingFixtureinstances. Each one contains a100vhSidePaneland dozens of components, including popup-capableSelectandTooltip. Enabling Chromatic here produces one very large snapshot that is likely to report diffs from any unrelated component change. Consider splitting the comparison into one story per theme, or setting adelayso async content settles before capture.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/react/common/stories/theming/ThemeComparison.stories.tsx` at line 41, Adjust the Chromatic configuration for the ThemeComparison story to reduce costly, unstable snapshots: either split the four-theme comparison into separate stories or configure an appropriate capture delay so asynchronous Select and Tooltip content settles before capture. Update the story containing the BrandingFixture instances while preserving the theme comparison coverage.modules/react/common/stories/mdx/examples/ThemingBrandScope.tsx (1)
21-28: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDrop the hardcoded
aria-selected.Line 21 sets
initialSelectedIds={['selected']}, so theMenumodel owns the selection state and setsaria-selectedon the matching item. The literalaria-selected="true"on Line 25 duplicates that, and it staystrueafter the user selects another item.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/react/common/stories/mdx/examples/ThemingBrandScope.tsx` around lines 21 - 28, Remove the hardcoded aria-selected="true" attribute from the Menu.Item with id "selected" in ThemingBrandScope, allowing Menu's selection model from initialSelectedIds to manage the attribute and update it when selection changes.utils/storybook/customThemes.ts (1)
91-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnnotate the preset type.
brandScopePrimaryOnlyandcustomNumericalThemedeclareCanvasNumericalBrandTheme.primaryWithFocushas no annotation, so a misspelled palette key stays undetected until a story renders. Add: CanvasProviderTheme.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@utils/storybook/customThemes.ts` around lines 91 - 99, Annotate the primaryWithFocus preset with the CanvasProviderTheme type, matching the existing type annotations used by brandScopePrimaryOnly and customNumericalTheme, so its canvas palette keys are validated.modules/react/common/spec/theming-types.spec.ts (1)
18-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
common.focusOutline-only branch.
resolveThemingScopehas a special rule: apalette.commonthat contains onlyfocusOutlinestays onbrandscope, and any othercommonkey promotes tofull(modules/react/common/lib/theming/types.tsLines 537-543). TheprimaryWithFocuspreset inutils/storybook/customThemes.tsdepends on that rule. Add two cases:common: {focusOutline: 'teal'}returns'brand', andcommon: {alertInner: 'coral'}returns'full'.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/react/common/spec/theming-types.spec.ts` around lines 18 - 48, Extend the resolveThemingScope tests to cover the palette.common rule: add a case where common contains only focusOutline and expect brand, plus a case where common contains alertInner and expect full. Use the existing resolveThemingScope test suite and preserve all current cases.utils/storybook/index.ts (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExport the new presets from the barrel.
brandScopePrimaryOnlyandprimaryWithFocusare not re-exported here.modules/react/common/stories/theming/ThemeComparison.stories.tsxLine 8 imports them through the deep relative path../../../../../utils/storybook/customThemes. Add both names to this export so consumers use one import path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@utils/storybook/index.ts` at line 7, Update the barrel export in the storybook utilities to include both brandScopePrimaryOnly and primaryWithFocus from customThemes, alongside the existing customColorTheme, customNumericalTheme, and toCanvasProviderTheme exports. This lets ThemeComparison.stories.tsx and other consumers import the presets from the barrel instead of using the deep relative path.modules/react/common/lib/theming/types.ts (1)
520-562: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant
themeScopecheck in the numerical branch.Line 524 already returns
theme.themeScopewhen it is set. Sotheme.themeScope ?? 'brand'at Line 529 can only evaluate to'brand'.♻️ Proposed simplification
if (isNumericalTheme(theme)) { - return theme.themeScope ?? 'brand'; + return 'brand'; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/react/common/lib/theming/types.ts` around lines 520 - 562, Remove the redundant theme.themeScope fallback inside the isNumericalTheme branch of resolveThemingScope, since the earlier themeScope check already handles any defined value. Preserve the numerical-theme default behavior by returning 'brand' directly from that branch.utils/storybook/CanvasProviderDecorator.tsx (1)
15-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
toCanvasProviderThemeinstead of always wrapping incanvas.Line 17 wraps
parameters.themein{canvas: ...}unconditionally. If a story supplies a numericalbrandtheme, the result is{canvas: {brand: ...}}, whichisNumericalThemeclassifies as semantic and which writes no brand variables.toCanvasProviderThemeinutils/storybook/customThemes.tsalready handles all three input shapes and is re-exported fromutils/storybook/index.ts.♻️ Proposed change
-import {CanvasProvider, PartialCanvasTheme} from '`@workday/canvas-kit-react/common`'; +import {CanvasProvider} from '`@workday/canvas-kit-react/common`'; + +import {toCanvasProviderTheme} from './customThemes';wrapper: (storyFn, context, {parameters = {}}) => ( <CanvasProvider - {...(parameters.theme ? {theme: {canvas: parameters.theme as PartialCanvasTheme}} : {})} + {...(parameters.theme ? {theme: toCanvasProviderTheme(parameters.theme)} : {})} className={storyStyles} >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@utils/storybook/CanvasProviderDecorator.tsx` around lines 15 - 22, Update the CanvasProviderDecorator wrapper to pass parameters.theme through the existing toCanvasProviderTheme helper instead of unconditionally nesting it under canvas. Preserve the conditional omission when no theme is provided, and use the helper’s normalized result so numerical, semantic, and canvas-shaped themes are handled correctly.modules/react/common/lib/theming/brandScope.ts (1)
371-399: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated
commontoken writer.Lines 373-399 repeat the logic in
writeIndependentBrandTokensat Lines 279-311. Both blocks iterate['focusOutline', 'alertInner', 'alertOuter', 'errorInner'], writebrand.common[key]andcommonTokenMapping[key], and then apply the same focus, caution, and critical side effects. The only difference is the default-value skip.Extract one helper that takes the key, the value, and a
skipDefaultsflag. That removes six@ts-ignorecomments and prevents the two copies from diverging further.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/react/common/lib/theming/brandScope.ts` around lines 371 - 399, Extract the duplicated common-token writing logic from the current color loop and writeIndependentBrandTokens into one shared helper, accepting the token key, value, and a skipDefaults flag. Keep the helper responsible for brand.common/commonTokenMapping writes and the existing focus, caution, and critical side effects; callers should only handle iteration and default-value filtering. Remove the duplicated implementation and its associated `@ts-ignore` comments while preserving skipDefaults behavior.modules/react/common/spec/brandScope.spec.ts (1)
85-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for the single-key primary shortcut.
The input on Lines 91-96 gives
primarytwo keys, soObject.keys(brandPalette.primary).length === 1is false andapplyPrimaryBrandBundlenever runs in this test. The shortcut path is the default path for the documented minimal theme. Add a case for{brand: {primary: {'600': 'purple'}}}, and a case that also setsaction: {base: 'teal'}to pin the precedence between the shortcut and an explicit action value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/react/common/spec/brandScope.spec.ts` around lines 85 - 109, Add coverage in the writeNumericalTheme tests for a minimal theme with primary containing only the 600 key, verifying the single-key shortcut applies; also add a case with action.base set to teal and assert the explicit action value takes precedence over the shortcut. Use the existing brand and system token assertions, keeping the current multi-ramp test unchanged.modules/react/popup/spec/usePopupStack.spec.tsx (1)
35-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the
if (styles)guards so the assertions cannot be skipped.
expect(container).toBeTruthy()already proves the container exists, so the guard is unreachable-by-design. InsidewaitFor, a guard that skips every assertion lets the callback resolve with no checks, so the test passes even when no variable was forwarded. The same pattern appears at lines 63, 97, 118, 156, and 187. Assert on a non-optional local instead.♻️ Proposed refactor for the first occurrence
const container = result.current.current; - expect(container).toBeTruthy(); + expect(container).not.toBeNull(); + const styles = container!.style; // Wait for the effect to apply styles await waitFor(() => { - // Check that the container has the correct CSS variables - const styles = container?.style; - if (styles) { - // The numerical theme should set brand variables - // Numerical themes use the key names directly (600, 700, etc.) - const primary600 = styles.getPropertyValue('--cnvs-brand-primary-600'); - const primary700 = styles.getPropertyValue('--cnvs-brand-primary-700'); - expect(primary600).toBe('`#123456`'); - expect(primary700).toBe('`#234567`'); - } + expect(styles.getPropertyValue('--cnvs-brand-primary-600')).toBe('`#123456`'); + expect(styles.getPropertyValue('--cnvs-brand-primary-700')).toBe('`#234567`'); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/react/popup/spec/usePopupStack.spec.tsx` around lines 35 - 46, Remove the `if (styles)` guards around CSS variable assertions in the popup stack tests, including the occurrences near the referenced locations. After the existing `expect(container).toBeTruthy()`, use a non-optional `container.style` local and keep the assertions unconditional so each `waitFor` callback always validates the forwarded variables.modules/react/common/spec/CanvasProvider.spec.tsx (1)
9-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the
data-themecleanup intoafterEachand narrow the negative assertions.Two test-isolation risks exist:
- Lines 26 and 57 remove
data-themeinside the test body. If an assertion above them throws, the attribute stays ondocument.documentElementand leaks into later tests.- Lines 39 and 55 assert that
console.warnwas never called. Any unrelated warning, such as theuseThemecontext warning, fails these tests. Assert the absence of the specific message instead.♻️ Proposed refactor
describe('CanvasProvider', () => { describe('console warnings', () => { + afterEach(() => { + document.documentElement.removeAttribute('data-theme'); + vi.restoreAllMocks(); + }); + it('should warn when sanaCanvasProviderTheme is used with global Sana theme', () => { @@ - // Cleanup - document.documentElement.removeAttribute('data-theme'); - consoleSpy.mockRestore(); }); @@ - expect(consoleSpy).not.toHaveBeenCalled(); - - consoleSpy.mockRestore(); + expect(consoleSpy).not.toHaveBeenCalledWith( + expect.stringContaining('You are passing sanaCanvasProviderTheme to CanvasProvider but') + ); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/react/common/spec/CanvasProvider.spec.tsx` around lines 9 - 59, Update the CanvasProvider tests to remove document.documentElement’s data-theme in a shared afterEach cleanup so failures cannot leak state between tests. In the negative cases, replace broad consoleSpy.not.toHaveBeenCalled assertions with checks that the specific sanaCanvasProviderTheme warning message was not emitted, while preserving the existing positive assertion.modules/docs/llm/theming.md (1)
133-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse descriptive link text instead of "here."
markdownlint (MD059) flags this link. Replace "View token documentation here" with a self-describing link, for example "View the Canvas Tokens documentation." This is shared with an identical link in
Theming.mdx— see the consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/docs/llm/theming.md` around lines 133 - 134, Update the token documentation link in the theming documentation to use descriptive link text instead of “here,” such as “Canvas Tokens documentation,” while preserving the existing URL and surrounding meaning.Source: Linters/SAST tools
modules/docs/mdx/16.0-UPGRADE-GUIDE.mdx (1)
150-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd "Default Scope Change for Legacy Themes" to the Table of Contents.
Same gap as the
.mdtwin: the TOC omits this subsection even though deeper headings are linked elsewhere. See the consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/docs/mdx/16.0-UPGRADE-GUIDE.mdx` around lines 150 - 156, Add “Default Scope Change for Legacy Themes” as a nested entry in the Table of Contents for the Sana Canvas Theme section, using the heading’s corresponding anchor and preserving the existing ordering of subsection links.README.md (1)
90-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify the "don't theme with CanvasProvider" note next to the new theme-prop example.
This new example shows
theme={sanaCanvasProviderTheme}onCanvasProvider. The next paragraph (unchanged) says not to useCanvasProviderto theme. Add a short qualifier to the note (for example, "except for the popup-forwarding case above") so readers do not think the two examples conflict.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 90 - 100, Update the existing “don’t theme with CanvasProvider” note in README.md to add a brief qualifier excluding the popup-forwarding case demonstrated with sanaCanvasProviderTheme, while preserving the note’s general guidance.modules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.md (1)
144-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd "Default Scope Change for Legacy Themes" to the Table of Contents.
The TOC lists "Scoped Theming" but skips its "Default Scope Change for Legacy Themes" subsection, even though the TOC links comparably deep headings elsewhere (for example "Delete Button Outline Variant"). This subsection documents a breaking behavior change (
themeScopedefault), so it deserves direct discoverability. The identical gap exists in16.0-UPGRADE-GUIDE.mdx— see the consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.md` around lines 144 - 150, Update the Table of Contents in both 16.0-UPGRADE-GUIDE.md and 16.0-UPGRADE-GUIDE.mdx by adding a nested “Default Scope Change for Legacy Themes” entry under “Scoped Theming,” using the subsection’s generated anchor.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/react/common/lib/CanvasProvider.tsx`:
- Around line 219-246: Update the CanvasProvider render flow around
useCanvasThemeToCssVars and the content div: remove the redundant ...props merge
that restores the raw consumer style, and spread only the remaining element
props. Apply the generated className and merged style after the spread so
consumer props cannot override the theme CSS variables while preserving the
existing CanvasBrandStyleContext behavior.
In `@modules/react/common/lib/theming/brandScope.ts`:
- Around line 489-494: Update the primary shortcut branch in the brand scope
logic around applyPrimaryBrandBundle so skipRamp.action excludes only action
keys absent from the user-provided brandPalette.action; preserve explicit action
values such as base, while still skipping undefined keys that are derived from
primary.
- Around line 529-549: Update writeBrandScopeSemantic to handle
palette.neutral.main by applying the existing neutral brand bundle or equivalent
neutral-ramp writer, preserving the current handling for primary, error, alert,
and success. Ensure legacy themes with only neutral.main produce the expected
brand.neutral variables, consistent with resolveThemingScope’s brand-scope
behavior for every semantic palette color.
In `@modules/react/common/lib/theming/sanaTheme.ts`:
- Around line 39-40: Update sanaCanvasNumericalTheme’s themeScope to brand so
writeNumericalTheme applies the documented Sana preset shortcut behavior, and
document the existing ramp-token coverage if needed.
In `@modules/react/common/lib/theming/types.ts`:
- Around line 481-491: Update isNumericalTheme to recognize themes that define
only the top-level direction or themeScope fields, including either key in its
discriminator alongside brand, system, and selected. Preserve the existing
undefined and canvas-theme checks so numerical themes are correctly routed
through CanvasProvider.
- Around line 295-319: Update CanvasBrandRamp to remove unsupported 150, 850,
and A150 keys from the general brand ramp, or restrict those keys to the neutral
ramp type matching EXTENDED_BRAND_TOKEN_MAP. Preserve supported keys and ensure
writeNumericalBrandRamp cannot resolve unsupported extended values for arbitrary
brand colors.
In `@modules/react/common/stories/mdx/examples/SimplifiedSanaSetup.tsx`:
- Line 12: Remove the sanaCanvasProviderTheme prop from CanvasProvider in
modules/react/common/stories/mdx/examples/SimplifiedSanaSetup.tsx:12 so the
example relies on the global Sana theme. In
modules/react/common/stories/theming/ThemeComparison.stories.tsx:20-30, retain
theme={sanaCanvasProviderTheme} only if the fixture intentionally demonstrates
scoped theming; in that case, add an explicit expected-warning note, otherwise
remove the prop there as well.
In `@modules/react/common/stories/mdx/examples/ThemingBrandScope.tsx`:
- Around line 9-19: Resolve the mismatch in ThemingBrandScope by aligning the
explanatory paragraph with the theme configuration: either remove the explicit
selected.surface and selected.fg values so the example demonstrates only the
primary shortcut, or revise the prose to state that selected overrides the
derived selection colors.
In `@modules/react/common/stories/mdx/Theming.mdx`:
- Around line 106-133: Update the no-access-to-html example in Theming.mdx by
removing the data-theme attribute from CanvasProvider and deleting the inline
Sana variables import, while retaining theme={sanaCanvasProviderTheme} and
leaving Sana import guidance in the root CSS section.
In `@modules/react/common/stories/theming/examples/BrandingFixture.tsx`:
- Around line 44-47: Update the columnStyles definition in BrandingFixture to
remove the fixed 100vw width and use flexible sizing, such as flex: 1 with an
appropriate minWidth, so all BrandingFixture instances can remain side by side
in ThemeComparison while preserving readable fixture dimensions.
In `@modules/react/testing/lib/StaticStates.tsx`:
- Around line 50-55: Remove the hardcoded data-theme="sana-canvas" attribute
from the CanvasProvider in StaticStates, allowing callers to opt in through the
existing elemProps spread while preserving all other provider props and
children.
In `@modules/react/text-input/stories/visualTesting.stories.tsx`:
- Around line 26-27: Update the TextInputStates theme prop type to use the same
input union accepted by toCanvasProviderTheme, including
PartialCanvasTheme/customColorTheme, while preserving the existing conversion
and StaticStates behavior.
In `@specs/2026-07-31-sana-theme-optional-design.md`:
- Around line 97-119: Update the migration-guide example in the v16 upgrade
guide by changing its outer markdown fence from three backticks to four,
including the matching closing fence, while leaving the nested three-backtick
tsx fences unchanged.
---
Nitpick comments:
In `@modules/docs/llm/theming.md`:
- Around line 133-134: Update the token documentation link in the theming
documentation to use descriptive link text instead of “here,” such as “Canvas
Tokens documentation,” while preserving the existing URL and surrounding
meaning.
In `@modules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.md`:
- Around line 144-150: Update the Table of Contents in both
16.0-UPGRADE-GUIDE.md and 16.0-UPGRADE-GUIDE.mdx by adding a nested “Default
Scope Change for Legacy Themes” entry under “Scoped Theming,” using the
subsection’s generated anchor.
In `@modules/docs/mdx/16.0-UPGRADE-GUIDE.mdx`:
- Around line 150-156: Add “Default Scope Change for Legacy Themes” as a nested
entry in the Table of Contents for the Sana Canvas Theme section, using the
heading’s corresponding anchor and preserving the existing ordering of
subsection links.
In `@modules/react/common/lib/theming/brandScope.ts`:
- Around line 371-399: Extract the duplicated common-token writing logic from
the current color loop and writeIndependentBrandTokens into one shared helper,
accepting the token key, value, and a skipDefaults flag. Keep the helper
responsible for brand.common/commonTokenMapping writes and the existing focus,
caution, and critical side effects; callers should only handle iteration and
default-value filtering. Remove the duplicated implementation and its associated
`@ts-ignore` comments while preserving skipDefaults behavior.
In `@modules/react/common/lib/theming/types.ts`:
- Around line 520-562: Remove the redundant theme.themeScope fallback inside the
isNumericalTheme branch of resolveThemingScope, since the earlier themeScope
check already handles any defined value. Preserve the numerical-theme default
behavior by returning 'brand' directly from that branch.
In `@modules/react/common/spec/brandScope.spec.ts`:
- Around line 85-109: Add coverage in the writeNumericalTheme tests for a
minimal theme with primary containing only the 600 key, verifying the single-key
shortcut applies; also add a case with action.base set to teal and assert the
explicit action value takes precedence over the shortcut. Use the existing brand
and system token assertions, keeping the current multi-ramp test unchanged.
In `@modules/react/common/spec/CanvasProvider.spec.tsx`:
- Around line 9-59: Update the CanvasProvider tests to remove
document.documentElement’s data-theme in a shared afterEach cleanup so failures
cannot leak state between tests. In the negative cases, replace broad
consoleSpy.not.toHaveBeenCalled assertions with checks that the specific
sanaCanvasProviderTheme warning message was not emitted, while preserving the
existing positive assertion.
In `@modules/react/common/spec/theming-types.spec.ts`:
- Around line 18-48: Extend the resolveThemingScope tests to cover the
palette.common rule: add a case where common contains only focusOutline and
expect brand, plus a case where common contains alertInner and expect full. Use
the existing resolveThemingScope test suite and preserve all current cases.
In `@modules/react/common/stories/mdx/examples/SimplifiedSanaSetup.tsx`:
- Line 8: Rename the exported component in SimplifiedSanaSetup.tsx from
SimplifiedSetup to SimplifiedSanaSetup, and update the corresponding MDX import
and references to use the new name.
In `@modules/react/common/stories/mdx/examples/ThemingBrandScope.tsx`:
- Around line 21-28: Remove the hardcoded aria-selected="true" attribute from
the Menu.Item with id "selected" in ThemingBrandScope, allowing Menu's selection
model from initialSelectedIds to manage the attribute and update it when
selection changes.
In `@modules/react/common/stories/theming/ThemeComparison.stories.tsx`:
- Line 41: Adjust the Chromatic configuration for the ThemeComparison story to
reduce costly, unstable snapshots: either split the four-theme comparison into
separate stories or configure an appropriate capture delay so asynchronous
Select and Tooltip content settles before capture. Update the story containing
the BrandingFixture instances while preserving the theme comparison coverage.
In `@modules/react/popup/spec/usePopupStack.spec.tsx`:
- Around line 35-46: Remove the `if (styles)` guards around CSS variable
assertions in the popup stack tests, including the occurrences near the
referenced locations. After the existing `expect(container).toBeTruthy()`, use a
non-optional `container.style` local and keep the assertions unconditional so
each `waitFor` callback always validates the forwarded variables.
In `@README.md`:
- Around line 90-100: Update the existing “don’t theme with CanvasProvider” note
in README.md to add a brief qualifier excluding the popup-forwarding case
demonstrated with sanaCanvasProviderTheme, while preserving the note’s general
guidance.
In `@utils/storybook/CanvasProviderDecorator.tsx`:
- Around line 15-22: Update the CanvasProviderDecorator wrapper to pass
parameters.theme through the existing toCanvasProviderTheme helper instead of
unconditionally nesting it under canvas. Preserve the conditional omission when
no theme is provided, and use the helper’s normalized result so numerical,
semantic, and canvas-shaped themes are handled correctly.
In `@utils/storybook/customThemes.ts`:
- Around line 91-99: Annotate the primaryWithFocus preset with the
CanvasProviderTheme type, matching the existing type annotations used by
brandScopePrimaryOnly and customNumericalTheme, so its canvas palette keys are
validated.
In `@utils/storybook/index.ts`:
- Line 7: Update the barrel export in the storybook utilities to include both
brandScopePrimaryOnly and primaryWithFocus from customThemes, alongside the
existing customColorTheme, customNumericalTheme, and toCanvasProviderTheme
exports. This lets ThemeComparison.stories.tsx and other consumers import the
presets from the barrel instead of using the deep relative path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e66a6f0-92f0-484d-814b-377eb53bf39c
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (38)
.storybook/preview.jsREADME.mdmodules/docs/llm/theming.mdmodules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.mdmodules/docs/mdx/16.0-UPGRADE-GUIDE.mdxmodules/docs/package.jsonmodules/labs-react/package.jsonmodules/preview-react/package.jsonmodules/react/common/lib/CanvasProvider.tsxmodules/react/common/lib/theming/README.mdmodules/react/common/lib/theming/brandScope.tsmodules/react/common/lib/theming/index.tsmodules/react/common/lib/theming/sanaTheme.tsmodules/react/common/lib/theming/types.tsmodules/react/common/spec/CanvasProvider.spec.tsxmodules/react/common/spec/brandScope.spec.tsmodules/react/common/spec/sanaTheme.spec.tsmodules/react/common/spec/theming-types.spec.tsmodules/react/common/spec/useCanvasThemeToCssVars.spec.tsxmodules/react/common/stories/mdx/Theming.mdxmodules/react/common/stories/mdx/examples/SimplifiedSanaSetup.tsxmodules/react/common/stories/mdx/examples/ThemingBrandScope.tsxmodules/react/common/stories/theming/ThemeComparison.stories.tsxmodules/react/common/stories/theming/examples/BrandingFixture.tsxmodules/react/package.jsonmodules/react/popup/lib/hooks/usePopupStack.tsmodules/react/popup/spec/usePopupStack.spec.tsxmodules/react/testing/lib/StaticStates.tsxmodules/react/text-area/stories/visualTesting.stories.tsxmodules/react/text-input/stories/visualTesting.stories.tsxmodules/styling-transform/package.jsonmodules/styling-transform/spec/utils/handleColorSpace.spec.tsmodules/styling/package.jsonpackage.jsonspecs/2026-07-31-sana-theme-optional-design.mdutils/storybook/CanvasProviderDecorator.tsxutils/storybook/customThemes.tsutils/storybook/index.ts
sheelah
reviewed
Jul 31, 2026
sheelah
reviewed
Jul 31, 2026
sheelah
reviewed
Jul 31, 2026
sheelah
reviewed
Jul 31, 2026
sheelah
reviewed
Jul 31, 2026
Resolve canvas-tokens-web version conflicts by keeping ^4.4.0. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
modules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.md (2)
318-325: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep the codemod notes in one blockquote.
The blank line at Line 324 is not prefixed with
>. It closes the blockquote and triggers MD028. Add a quoted blank line before theNoteparagraph.Proposed fix
> the [Icon Migration codemod guide](/docs/guides-icon-migration-codemod--docs). +> > **Note:** 🤖 The `icon-migration` codemod automates the majority of system icon migration work,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.md` around lines 318 - 325, Keep the codemod guidance and the Note paragraph within one continuous blockquote by prefixing the blank separator line before the Note with `>`. Preserve the existing text and formatting otherwise.Source: Linters/SAST tools
275-281: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the v16 codemod instructions consistent.
This section lists only
updateHyperlinkProps, but the later v16 codemod section listsupdateCardVariantandupdateSidePanelVarianttoo. Users may miss required migrations. Add the missing transformations here or keep one canonical section.Proposed fix
## Codemod Transformations for v16 @@ +- **Update Card Variant**: updateCardVariant - **Update Hyperlink Props**: updateHyperlinkProps +- **Update Side Panel Variant**: updateSidePanelVariant🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.md` around lines 275 - 281, Update the “Codemod Transformations for v16” section to include updateCardVariant and updateSidePanelVariant alongside updateHyperlinkProps, matching the later v16 codemod section. Keep the codemod command and existing transformation entry unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/docs/utils/parseSpecFile.ts`:
- Around line 40-66: Update the import-replacement logic in parseSpecFile to
capture the module specifier and parse default, namespace, and named bindings
independently, including mixed forms such as default-plus-named imports. Base
React-specific stubbing on the module path, preserve valid identifiers for each
binding, and avoid emitting invalid declarations for default-plus-namespace
imports. Add fixtures covering the affected valid import combinations.
In `@modules/react/popup/lib/hooks/usePopupStack.ts`:
- Around line 78-87: Update the style-forwarding effect in usePopupStack to
track the CSS variable keys applied during the previous run, remove any keys
missing from the next style context before applying current string values, and
preserve the existing values on unmount if required for flash-free reopening.
---
Outside diff comments:
In `@modules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.md`:
- Around line 318-325: Keep the codemod guidance and the Note paragraph within
one continuous blockquote by prefixing the blank separator line before the Note
with `>`. Preserve the existing text and formatting otherwise.
- Around line 275-281: Update the “Codemod Transformations for v16” section to
include updateCardVariant and updateSidePanelVariant alongside
updateHyperlinkProps, matching the later v16 codemod section. Keep the codemod
command and existing transformation entry unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e6a63f8-cd52-4e71-9665-adb4172701e0
📒 Files selected for processing (23)
README.mdmodules/docs/llm/theming.mdmodules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.mdmodules/docs/mdx/16.0-UPGRADE-GUIDE.mdxmodules/docs/tsconfig.es6.jsonmodules/docs/tsconfig.jsonmodules/docs/utils/build-specifications.jsmodules/docs/utils/parseSpecFile.tsmodules/react/common/lib/CanvasProvider.tsxmodules/react/common/lib/theming/brandScope.tsmodules/react/common/lib/theming/sanaTheme.tsmodules/react/common/lib/theming/types.tsmodules/react/common/spec/brandScope.spec.tsmodules/react/common/spec/theming-types.spec.tsmodules/react/common/stories/mdx/Theming.mdxmodules/react/common/stories/mdx/examples/SimplifiedSanaSetup.tsxmodules/react/common/stories/mdx/examples/ThemingBrandScope.tsxmodules/react/common/stories/theming/ThemeComparison.stories.tsxmodules/react/common/stories/theming/examples/BrandingFixture.tsxmodules/react/popup/lib/hooks/usePopupStack.tsmodules/react/popup/spec/usePopupStack.spec.tsxmodules/react/testing/lib/StaticStates.tsxmodules/react/text-input/stories/visualTesting.stories.tsx
🚧 Files skipped from review as they are similar to previous changes (13)
- modules/react/common/spec/theming-types.spec.ts
- modules/react/testing/lib/StaticStates.tsx
- modules/react/common/lib/theming/sanaTheme.ts
- modules/react/common/stories/mdx/examples/ThemingBrandScope.tsx
- README.md
- modules/react/text-input/stories/visualTesting.stories.tsx
- modules/docs/llm/theming.md
- modules/react/common/lib/theming/types.ts
- modules/react/common/stories/theming/ThemeComparison.stories.tsx
- modules/react/popup/spec/usePopupStack.spec.tsx
- modules/docs/mdx/16.0-UPGRADE-GUIDE.mdx
- modules/react/common/stories/theming/examples/BrandingFixture.tsx
- modules/react/common/lib/theming/brandScope.ts
Comment on lines
+40
to
+66
| /import\s+([\s\S]+?)\s+from\s+['"][^'"]+['"];?/g, | ||
| (_substr: string, imports: string) => { | ||
| const trimmed = imports.trim(); | ||
| if (trimmed.includes('{')) { | ||
| const names = trimmed | ||
| .replace(/[{}]/g, '') | ||
| .split(',') | ||
| .map(part => { | ||
| // support `Foo as Bar` and whitespace/newlines | ||
| const pieces = part | ||
| .trim() | ||
| .split(/\s+as\s+|\s+/) | ||
| .filter(Boolean); | ||
| return pieces[pieces.length - 1]; | ||
| }) | ||
| .filter(Boolean); | ||
| // Stub as both a component and a CSF story object (`Example.render`) | ||
| return names | ||
| .map(name => `const ${name} = Object.assign(() => {}, {render: () => {}});`) | ||
| .join('\n'); | ||
| } | ||
| if (/react/i.test(trimmed)) { | ||
| return `const React = {createElement: () => {}};`; | ||
| } | ||
| // default / namespace imports — stub the binding name | ||
| const name = trimmed.replace(/^\*\s+as\s+/, '').trim(); | ||
| return name ? `const ${name} = () => {};` : ''; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python - <<'PY'
from pathlib import Path
import re
mixed = re.compile(
r'(?m)^\s*import\s+[A-Za-z_$][\w$]*\s*,\s*'
r'(?:\{[\s\S]*?\}|\*\s+as\s+[A-Za-z_$][\w$]*)\s+from\s+'
r'["\']([^"\']+)["\']'
)
default = re.compile(
r'(?m)^\s*import\s+([A-Za-z_$][\w$]*)\s+from\s+["\']([^"\']+)["\']'
)
found = False
for path in Path(".").rglob("*"):
if path.suffix not in {".ts", ".tsx"}:
continue
if any(part in {".git", "node_modules", "dist"} for part in path.parts):
continue
text = path.read_text(encoding="utf-8", errors="ignore")
for match in mixed.finditer(text):
found = True
print(f"mixed import: {path}: {match.group(1)}")
for binding, module in default.findall(text):
if "react" in binding.lower() and module.lower() != "react":
found = True
print(f"binding/module mismatch: {path}: {binding} from {module}")
if not found:
print("No affected import forms found.")
PYRepository: Workday/canvas-kit
Length of output: 4059
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== files =="
fd -a 'parseSpecFile\.ts|get-specifications\.js' . | sed 's#^\./##'
echo "== parseSpecFile outline/size =="
wc -l modules/docs/utils/parseSpecFile.ts || true
ast-grep outline modules/docs/utils/parseSpecFile.ts || true
echo "== parseSpecFile relevant lines =="
cat -n modules/docs/utils/parseSpecFile.ts | sed -n '1,150p'
echo "== get-specifications relevant lines =="
if [ -f modules/docs/lib/get-specifications.js ]; then
wc -l modules/docs/lib/get-specifications.js
cat -n modules/docs/lib/get-specifications.js | sed -n '1,220p'
fi
echo "== deterministic import parser matcher examples =="
node - <<'JS'
const re = /import\s+([\s\S]+?)\s+from\s+['"][^'"]+['"];?/g;
const cases = [
"import React, { useState } from 'react';",
"import ReactDOM from 'react-dom';",
"import Foo, * as utils from 'foo';",
"import React from 'react';",
"import * as Utils from 'foo';",
"import React, { useState } as Example from 'react-dom';",
];
for (const input of cases) {
re.lastIndex = 0;
const m = re.exec(input);
if (!m) {
console.log(input, "NO MATCH");
continue;
}
const imports = m[1];
const trimmed = imports.trim();
console.log("INPUT:", input);
console.log("captured clause:", JSON.stringify(imports));
if (trimmed.includes('{')) {
const names = trimmed
.replace(/[{}]/g, '')
.split(',')
.map(part => {
const pieces = part.trim().split(/\s+as\s+|\s+/).filter(Boolean);
return pieces[pieces.length - 1];
})
.filter(Boolean);
console.log("named branch names:", JSON.stringify(names));
console.log("replacement:", names.map(name => `const ${name} = Object.assign(() => {}, {render: () => {}});`).join('\n'));
} else if (/react/i.test(trimmed)) {
console.log("react branch replacement:", "const React = {createElement: () => {}};");
} else {
const name = trimmed.replace(/^\*\s+as\s+/, '').trim();
console.log("default/namespace branch replacement:", name ? `const ${name} = () => {};` : '');
}
console.log();
}
JSRepository: Workday/canvas-kit
Length of output: 7186
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== parseSpecFile tests/usages =="
rg -n "parseSpecFile|MissingFileError|getSpec|spec" modules/docs -g '*.ts' -g '*.tsx' -g '*.js' | head -n 200
echo "== get-specifications context =="
cat -n modules/docs/utils/get-specifications.js | sed -n '1,260p'
echo "== package/typescript availability =="
node - <<'JS'
try {
const ts = require('typescript');
console.log('typescript present', ts.version);
const examples = [
"import React, { useState } from 'react';\nconst x = <div>{React.createElement('div')}</div>;\n",
"import ReactDOM from 'react-dom';\nconst x = <div/>;\n",
"import Foo, * as utils from 'foo';\n",
];
for (const src of examples) {
console.log("---");
console.log(src);
try {
const out = ts.transpile(src, {jsx: ts.JsxEmit.React, target: ts.ScriptTarget.ES2015});
console.log(out);
} catch (e) {
console.log(String(e && !(e instanceof Error) ? e : e.toString()));
}
}
} catch (e) {
console.log('typescript not available');
console.log(String(e && !(e instanceof Error) ? e : e.toString()));
}
JSRepository: Workday/canvas-kit
Length of output: 6797
Parse import bindings by module and import form.
The regex captures the entire import clause, but the replacements only inspect the binding text. For import React, { useState } from 'react';, both React and useState enter the named-import branch, so JSX transpilation emits React.createElement, but React becomes a function stub instead of an object with createElement. This throws at eval(contents) and makes get-specifications.js skip the spec with a warning.
The parser also emits invalid declarations for default-plus-namespace imports and generates const React for import ReactDOM from 'react-dom'; because the React branch checks the binding instead of the module specifier.
Capture the module specifier and parse default, namespace, and named bindings separately. Add fixtures for the affected valid import forms.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/docs/utils/parseSpecFile.ts` around lines 40 - 66, Update the
import-replacement logic in parseSpecFile to capture the module specifier and
parse default, namespace, and named bindings independently, including mixed
forms such as default-plus-named imports. Base React-specific stubbing on the
module path, preserve valid identifiers for each binding, and avoid emitting
invalid declarations for default-plus-namespace imports. Add fixtures covering
the affected valid import combinations.
Comment on lines
+78
to
+87
| const styleKeys = Object.keys(style).filter(key => key.startsWith('--')); | ||
| if (styleKeys.length === 0) { | ||
| return undefined; | ||
| } | ||
| for (const key of styleKeys) { | ||
| // @ts-ignore - token keys are CSS custom property names | ||
| element.style.setProperty(key, style[key]); | ||
| const value = style[key as keyof typeof style]; | ||
| if (typeof value !== 'string') { | ||
| continue; | ||
| } | ||
| element.style.setProperty(key, value); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove CSS variables that are no longer forwarded.
When style loses a CSS variable, this effect leaves the previous inline value on the popup container. The stale value overrides inherited variables after a live theme or scope change.
Track the previously applied keys. Remove keys that are absent from the next context value before setting the current values. Keep the current values on unmount if reopening without a flash is required.
Proposed fix
+ const appliedStyleKeysRef = React.useRef(new Set<string>());
+
React.useLayoutEffect(() => {
const element = localRef.current;
if (!element) {
return undefined;
}
- const styleKeys = Object.keys(style).filter(key => key.startsWith('--'));
- if (styleKeys.length === 0) {
- return undefined;
- }
- for (const key of styleKeys) {
+ const nextStyleKeys = new Set<string>();
+ for (const key of Object.keys(style).filter(key => key.startsWith('--'))) {
const value = style[key as keyof typeof style];
if (typeof value !== 'string') {
continue;
}
+ nextStyleKeys.add(key);
element.style.setProperty(key, value);
}
+ for (const key of appliedStyleKeysRef.current) {
+ if (!nextStyleKeys.has(key)) {
+ element.style.removeProperty(key);
+ }
+ }
+ appliedStyleKeysRef.current = nextStyleKeys;
return undefined;
}, [localRef, style]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const styleKeys = Object.keys(style).filter(key => key.startsWith('--')); | |
| if (styleKeys.length === 0) { | |
| return undefined; | |
| } | |
| for (const key of styleKeys) { | |
| // @ts-ignore - token keys are CSS custom property names | |
| element.style.setProperty(key, style[key]); | |
| const value = style[key as keyof typeof style]; | |
| if (typeof value !== 'string') { | |
| continue; | |
| } | |
| element.style.setProperty(key, value); | |
| const appliedStyleKeysRef = React.useRef(new Set<string>()); | |
| React.useLayoutEffect(() => { | |
| const element = localRef.current; | |
| if (!element) { | |
| return undefined; | |
| } | |
| const nextStyleKeys = new Set<string>(); | |
| for (const key of Object.keys(style).filter(key => key.startsWith('--'))) { | |
| const value = style[key as keyof typeof style]; | |
| if (typeof value !== 'string') { | |
| continue; | |
| } | |
| nextStyleKeys.add(key); | |
| element.style.setProperty(key, value); | |
| } | |
| for (const key of appliedStyleKeysRef.current) { | |
| if (!nextStyleKeys.has(key)) { | |
| element.style.removeProperty(key); | |
| } | |
| } | |
| appliedStyleKeysRef.current = nextStyleKeys; | |
| return undefined; | |
| }, [localRef, style]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/react/popup/lib/hooks/usePopupStack.ts` around lines 78 - 87, Update
the style-forwarding effect in usePopupStack to track the CSS variable keys
applied during the previous run, remove any keys missing from the next style
context before applying current string values, and preserve the existing values
on unmount if required for flash-free reopening.
10 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
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.
Summary
Updates
CanvasProviderscoped theming for v16 / Sana Canvas: a numericalbrandtheme API, explicitthemeScope('brand'|'full'), Sana presets for popup parity, and a fix so portaled popups inherit the correct brand CSS variables.Release Category
Components
Release Note
CanvasProvidernow prefers a numericalbrandtheme shape (e.g.{brand: {primary: {'600': '...'}}}) for scoped theming. UsesanaCanvasProviderThemeat the root provider when global Sana CSS is loaded so menus, selects, modals, and other popups get the same brand variables. Popup forwarding no longer relies on Emotion’s legacy theme context, so numerical themes no longer incorrectly force classic Canvas defaults onto popup containers.BREAKING CHANGES
Legacy themes that only set
canvas.palette.<color>.mainnow default tothemeScope: 'brand'(PrimaryButton + selected states) instead of the previous implicit full ramp / broadsystem.color.brand.*forwarding. To restore v15 behavior, passthemeScope: 'full':Checklist
ready for reviewhas been added to PRFor the Reviewer
Where Should the Reviewer Start?
Areas for Feedback? (optional)
Testing Manually
Screenshots or GIFs (if applicable)
Thank You Gif (optional)
Summary by CodeRabbit
New Features
CanvasProviderconfiguration, including brand and full semantic theme scopes.Bug Fixes
Tests