fix(ui-scripts,emotion): fix primitives and semantics override functionality by HerrTopi · Pull Request #2495 · instructure/instructure-ui

HerrTopi · 2026-03-30T13:48:38Z

The PR converts the theme system from static token objects to lazy functions (semantics(primitives), components(semantics), sharedTokens(semantics)), enabling override-by-recomputation. It also
introduces frozen themes so older component version's themes stay pinned to a snapshot.

This solution stores the primitives, semantics, sharedTokens (every part of every theme, except the unaffected components) and the affected components, for example packages/ui-themes/src/themes/frozenThemes/v11_7/dark/primitives.ts

TEST_PLAN:

Check if overrides in 11.6 have not changed.
Check the /v11_7/new-theme-overrides to see if it works as it should
Check the code logic especially for InstUISettingsProvider and withStyle

Notes:

To test the frozen theme system, please check out [https://github.com/feat(ui-themes,ui-alerts): Test PR on how theme freezing works #2519](This pr). It's the same as this but with added frozen functionality for Alert. It also contains some examples. That's only a show-case pr, will be abandoned when this will be merged

TODO for later: Find ALL v2 components that use themeOverride internally and migrate to this new system. (e.g. ColorPreset, Select)

github-actions · 2026-03-30T21:52:45Z

PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-04-23 12:57 UTC

matyasf · 2026-04-15T09:38:25Z

TODO:

there should be just 1 colors.ts: import { colors } from '../canvas/colors'
comment in the new themes:
// This is needed so 11.6 component don't break when using this theme.
// They will default to the 'canvas' theme
...sharedThemeTokens,
colors
update description "Only usable with v11.7 or newer components"
sharedThemeTokens -> legacySharedThemeTokens
newThemes -> newThemeTokens

ToMESSKa · 2026-04-17T14:30:33Z

This pill is not purple

ToMESSKa · 2026-04-17T14:33:21Z

This button is not deep pink

ToMESSKa · 2026-04-17T15:00:50Z

+<InstUISettingsProvider theme={canvas}>
+  <Alert variant="info" margin="small">
+    Default info Alert.
+  </Alert>
+  <Alert
+    variant="info"
+    margin="small"
+    themeOverride={{
+      infoBorderColor: 'crimson',
+      infoIconBackground: 'crimson'
+    }}
+  >
+    This specific Alert has crimson info styling.
+  </Alert>
+</InstUISettingsProvider>


This example might give someone the false impression that InstUISettingsProvider wrapper is needed around the component when using the themeOverride prop (which is not)?

Good catch. removed.

ToMESSKa

see my comments

matyasf · 2026-04-20T10:46:31Z


 export type {
  BaseThemeOrOverride,
-  ThemeOrOverride,


Technically this is a breaking change, but I think we can leave it in

matyasf

Looks good, please add a task to write unit tests for this functionality

git-nandor · 2026-04-20T17:54:29Z

+        },
+        Pill: {
+          primaryBackground: 'purple',
+          primaryBorderColor: 'purple'


The variable names for the pill are incorrect. Please use backgroundColor and baseBorderColor instead.

git-nandor · 2026-04-20T21:26:22Z

+          Pill:{
+
+          }
+        }


Is the double InstUISettingsProvider wrapper intentional?
Was it intentional to modify this old documentation page?

Copilot

Pull request overview

Copilot reviewed 138 out of 151 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (2)

packages/ui-scripts/lib/build/buildThemes/setupThemes.js:257

The theme index template declares export type Theme = BaseTheme<Primitives, Semantics> but then emits const theme: any = { ... }. Using any here removes most of the value of generating the types and makes it easy for token shape regressions to slip through. Prefer emitting const theme: Theme = ... and fix/cast only the specific fields that currently don’t type-check.
packages/emotion/src/getComponentThemeOverride.ts:56
getComponentThemeOverride now types its theme parameter as ThemeOverride, but all call sites (e.g. withStyleLegacy, useStyleLegacy, and tests) pass the value returned from useTheme() which is BaseThemeOrOverride (and often a full theme like canvas). This looks like it will create TypeScript assignability errors. The parameter type should likely stay as BaseThemeOrOverride/ThemeOrLegacyOverride (or be widened) rather than the legacy override-only type.

import type {
  ThemeOverride,
  Overrides,
  ComponentOverride
} from './EmotionTypes'
import type { ComponentTheme } from '@instructure/shared-types'
import { ThemeOverrideProp } from './withStyle'
import { ThemeOverrideValue } from './useStyle'

type ComponentName = keyof ComponentOverride | undefined

/**
 * ---
 * private: true
 * ---
 * This is a utility function which calculates the correct component theme
 * based on every possible override there is.

 * @param theme - Theme object
 * @param displayName - Name of the component
 * @param componentId - componentId of the component
 * @param themeOverride - The theme override object
 * @param componentTheme - The component's default theme
 * @returns The calculated theme override object
 */
const getComponentThemeOverride = (
  theme: ThemeOverride,
  displayName: string,
  componentId?: string,
  // ThemeOverrideProp is the old type, ThemeOverrideValue is the new one
  themeOverride?: ThemeOverrideProp['themeOverride'] | ThemeOverrideValue,
  componentTheme?: ComponentTheme

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-21T13:24:56Z

        import { SharedTokens } from '../commonTypes'
+        import { Semantics } from "./semantics"



In the generated sharedTokens.ts content you import Semantics via import { Semantics } from "./semantics", but Semantics is a type export. This should be a type-only import to avoid runtime imports / TS errors under stricter module settings (and it’s already done as import type { Semantics } ... elsewhere in this generator).

Copilot · 2026-04-21T13:24:56Z

    <InstUISettingsProvider
-      theme={{
-        componentOverrides: {
+      themeOverride={{
+        components:{
          Alert: {
            infoIconBackground: "darkblue",
            infoBorderColor: "darkblue"
+          },
+          Pill:{
+
+          }
+        }
+      }}
+    >
+    <InstUISettingsProvider
+      themeOverride={{
+        components:{
+          Alert: {
+            infoIconBackground: "darkblue",
+            infoBorderColor: "darkblue"
+          },
+          Pill:{
+            infoTextColor:"red"
          }
        }
      }}
    >
      <Alert variant="info" margin="small">
        My icon background and border should be dark blue in any theme.
      </Alert>
+       <Pill
+    statusLabel="Status"
+    color="info"
+    margin="x-small"
+  >
+    Draft
+  </Pill>
+    </InstUISettingsProvider>
    </InstUISettingsProvider>


This guide section is titled/structured around the legacy theme={{ componentOverrides: ... }} API, but the updated example uses nested InstUISettingsProvider themeOverride={...} blocks, repeats the same Alert override twice, and includes an empty Pill: {} override. This makes the example hard to follow and suggests a no-op override. Consider simplifying to a single provider (or clearly demonstrating nesting) and remove the empty/duplicated override entries.

Copilot · 2026-04-21T13:24:56Z

      theme={{
        componentOverrides: {
-          TextInput: { backgroundColor: "yellow" }
+          TextInput: { background: "yellow" }


In this example the componentOverrides entry for TextInput uses { background: "yellow" }, but the TextInput theme variables in this repo use backgroundColor / backgroundHoverColor / etc. As written, the override is likely a no-op. Update the example to use the correct token key so readers can copy/paste it successfully.

Suggested change

TextInput: { background: "yellow" }

TextInput: { backgroundColor: "yellow" }

Copilot · 2026-04-21T13:24:57Z

+    const componentId =
+      useTokensFrom ?? ComposedComponent.componentId?.replace('.', '')



componentId can end up undefined when withStyle is used on a component without componentId and without useTokensFrom. That causes lookups like themeOverride?.components?.[componentId] and theme.newTheme.components[componentId]?.(...) to use the string key 'undefined', and can lead to styles being computed with an empty theme unexpectedly. Consider validating componentId (and failing fast in dev) or providing a safe fallback (e.g., require componentId / useTokensFrom for this codepath).

Suggested change

const componentId =

useTokensFrom ?? ComposedComponent.componentId?.replace('.', '')

const resolvedComponentId =

useTokensFrom ?? ComposedComponent.componentId?.replace('.', '')

if (

!resolvedComponentId &&

typeof process !== 'undefined' &&

process?.env?.NODE_ENV !== 'production'

) {

warn(

false,

`[withStyle] ${displayName || 'Component'} must define either componentId or useTokensFrom. Falling back to an empty component id to avoid invalid theme lookups.`

)

}

const componentId = resolvedComponentId ?? ''

Copilot · 2026-04-21T13:24:57Z

+      // if a new theme has been added to the lib since this component version has been frozen, it can't be used with this
+      // theme, so we throw an error. Solution: upgrade to the latest version, it will support it
+      if (frozenTheme && !frozenTheme[themeKey]) {
+        console.error(
+          `The version of ${displayName} you are using does not support the currently applied "${themeKey}" theme. ` +
+          `Please upgrade to the latest version of ${displayName}.`
+        )
+      }


The comment says we "throw an error" when a frozen component version doesn't support the applied theme, but the code only calls console.error and continues rendering. This can both spam logs (it runs on every render) and leave the UI in an undefined state. Either actually throw (or warn(false, ...) once) or update the comment/behavior so it's consistent and non-noisy (e.g., gate behind dev-only and/or de-dupe).

…onality

note: its not complete, the WithStyleProps still has wrong types in most v2 components

…ides and add freeze functionlionality to old components

HerrTopi changed the title ~~fix(ui-scripts,emotion): fix primitives and semantics override functi…~~ fix(ui-scripts,emotion): fix primitives and semantics override functionality Mar 30, 2026

matyasf self-requested a review March 30, 2026 14:30

matyasf assigned HerrTopi Mar 30, 2026

HerrTopi force-pushed the fix-theme-overrides branch from 4e09919 to 348cc7a Compare April 4, 2026 19:07

matyasf force-pushed the fix-theme-overrides branch from 348cc7a to 7b25cc6 Compare April 8, 2026 13:06

matyasf reviewed Apr 8, 2026

View reviewed changes

Comment thread docs/guides/new-theme-overrides.md Outdated

This comment was marked as outdated.

Sign in to view

matyasf requested a review from balzss April 9, 2026 08:39

matyasf reviewed Apr 9, 2026

View reviewed changes

Comment thread .fallow/cache.bin Outdated

matyasf requested a review from Copilot April 9, 2026 08:50

Copilot started reviewing on behalf of matyasf April 9, 2026 08:51 View session

This comment was marked as resolved.

Sign in to view

HerrTopi force-pushed the fix-theme-overrides branch 2 times, most recently from 196adf1 to 07b822c Compare April 16, 2026 10:02

HerrTopi requested review from ToMESSKa, git-nandor, joyenjoyer and matyasf April 16, 2026 10:22

HerrTopi force-pushed the fix-theme-overrides branch from 07b822c to 2250600 Compare April 16, 2026 12:52

ToMESSKa reviewed Apr 17, 2026

View reviewed changes

ToMESSKa requested changes Apr 17, 2026

View reviewed changes

matyasf reviewed Apr 20, 2026

View reviewed changes

matyasf approved these changes Apr 20, 2026

View reviewed changes

Comment thread packages/emotion/src/useStyle.ts

Comment thread packages/emotion/src/withStyle.tsx

git-nandor requested changes Apr 20, 2026

View reviewed changes

HerrTopi force-pushed the fix-theme-overrides branch from 2250600 to 6623b92 Compare April 21, 2026 11:50

HerrTopi requested a review from git-nandor April 21, 2026 13:14

HerrTopi requested review from ToMESSKa and Copilot April 21, 2026 13:14

Copilot started reviewing on behalf of HerrTopi April 21, 2026 13:15 View session

Copilot AI reviewed Apr 21, 2026

View reviewed changes

HerrTopi and others added 6 commits April 21, 2026 16:12

fix(ui-scripts,emotion): fix primitives and semantics override functi…

945f9fd

…onality

refactor(many): fix TS types

3c0635e

note: its not complete, the WithStyleProps still has wrong types in most v2 components

refactor(ui-scripts): more theme typing

5bf0c7e

feat(ui-themes,ui-alerts,emotion): fix primitives and semantics overr…

8fcbde3

…ides and add freeze functionlionality to old components

refactor(many): fix types after rebase

eec94e3

feat(many): fix issues

e0c9869

HerrTopi force-pushed the fix-theme-overrides branch from 6623b92 to e0c9869 Compare April 21, 2026 14:31

ToMESSKa approved these changes Apr 22, 2026

View reviewed changes

HerrTopi merged commit 9d623ed into master Apr 23, 2026
8 of 9 checks passed

HerrTopi deleted the fix-theme-overrides branch April 23, 2026 12:57

+                        Pill:{
+                        }
+                      }

		import { SharedTokens } from '../commonTypes'
		import { Semantics } from "./semantics"

	TextInput: { background: "yellow" }
	TextInput: { backgroundColor: "yellow" }

		const componentId =
		useTokensFrom ?? ComposedComponent.componentId?.replace('.', '')

-    const componentId =
-      useTokensFrom ?? ComposedComponent.componentId?.replace('.', '')
+    const resolvedComponentId =
+      useTokensFrom ?? ComposedComponent.componentId?.replace('.', '')
+    if (
+      !resolvedComponentId &&
+      typeof process !== 'undefined' &&
+      process?.env?.NODE_ENV !== 'production'
+    ) {
+      warn(
+        false,
+        `[withStyle] ${displayName || 'Component'} must define either componentId or useTokensFrom. Falling back to an empty component id to avoid invalid theme lookups.`
+      )
+    }
+    const componentId = resolvedComponentId ?? ''

Conversation

HerrTopi commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

matyasf commented Apr 15, 2026

Uh oh!

ToMESSKa commented Apr 17, 2026

Uh oh!

ToMESSKa commented Apr 17, 2026

Uh oh!

ToMESSKa Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

HerrTopi Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

ToMESSKa left a comment

Choose a reason for hiding this comment

Uh oh!

matyasf Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

matyasf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

git-nandor Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

git-nandor Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

HerrTopi commented Mar 30, 2026 •

edited

Loading

github-actions Bot commented Mar 30, 2026 •

edited

Loading