feat: add app.branding config for portal rebranding - #708
Conversation
resolveBrandTokens + ChoreoTokensProvider in the design system; frontend- visible app.branding.* schema (name, logos, per-mode primary color) wired into portal-app themes, sidebar logos, and sign-in card. No branding config = byte-identical stock look. Part of openchoreo/openchoreo#4330 Signed-off-by: Kavith Lokuhewage <kaviththiranga@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughChangesApp branding
Backend composition
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ConfigAPI
participant useBranding
participant BrandedThemeProvider
participant resolveBrandTokens
participant ChoreoTokensProvider
participant useChoreoTokens
ConfigAPI->>useBranding: read app.branding
useBranding-->>BrandedThemeProvider: BrandingConfig
BrandedThemeProvider->>resolveBrandTokens: resolve mode primary color
resolveBrandTokens-->>BrandedThemeProvider: ThemeTokens
BrandedThemeProvider->>ChoreoTokensProvider: provide resolved tokens
ChoreoTokensProvider-->>useChoreoTokens: context token set
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/portal-app/src/themes.test.tsx (1)
21-53: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the dark branding path end to end.
These tests only cover
theme.light.primaryColor. Renderopenchoreo-darkwith distinct light/dark values and assert both palette andgraph.edgeuse the dark value; otherwise a provider that reads the light setting for both modes would pass.Based on PR objectives, branding supports independent light and dark primary colors.
🤖 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 `@packages/portal-app/src/themes.test.tsx` around lines 21 - 53, Add coverage for the dark branding path alongside the existing appThemes tests: render the openchoreo-dark Provider with distinct light and dark primaryColor configuration values, then assert both the dark palette primary and graph edge outputs use the dark value. Ensure the test would fail if the dark Provider incorrectly reads the light branding setting.
🤖 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 `@packages/design-system/src/theme/brand.ts`:
- Around line 61-104: Update the brand token generation around the theme’s
text.link and light-mode gradient values so supported brand colors such as
`#0d9488` produce WCAG AA-safe contrast for normal links and white header text.
Derive appropriate semantic variants or reject invalid colors, and add theme
coverage for the teal configuration while preserving existing dark-mode
behavior.
In `@packages/portal-app/config.d.ts`:
- Around line 34-51: Update the theme configuration handling associated with
light and dark primaryColor so values are validated or normalized during config
parsing before MUI v4 shade derivation; support only formats the color
manipulation accepts (hex, rgb, and hsl), or revise the schema and documentation
to explicitly restrict the accepted formats instead of claiming any CSS color.
---
Nitpick comments:
In `@packages/portal-app/src/themes.test.tsx`:
- Around line 21-53: Add coverage for the dark branding path alongside the
existing appThemes tests: render the openchoreo-dark Provider with distinct
light and dark primaryColor configuration values, then assert both the dark
palette primary and graph edge outputs use the dark value. Ensure the test would
fail if the dark Provider incorrectly reads the light branding setting.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 31a97851-37eb-4598-88d7-028da041d64f
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (18)
.changeset/app-branding-groundwork.mdpackages/design-system/src/index.tspackages/design-system/src/theme/ChoreoTokensProvider.tsxpackages/design-system/src/theme/brand.test.tspackages/design-system/src/theme/brand.tspackages/design-system/src/theme/useChoreoTokens.test.tsxpackages/design-system/src/theme/useChoreoTokens.tspackages/portal-app/config.d.tspackages/portal-app/package.jsonpackages/portal-app/src/branding.test.tspackages/portal-app/src/branding.tspackages/portal-app/src/components/DynamicSignInPage.tsxpackages/portal-app/src/components/Root/LogoFull.test.tsxpackages/portal-app/src/components/Root/LogoFull.tsxpackages/portal-app/src/components/Root/LogoIcon.tsxpackages/portal-app/src/index.tspackages/portal-app/src/themes.test.tsxpackages/portal-app/src/themes.tsx
| text: { | ||
| ...base.text, | ||
| link: main, | ||
| linkHover: isDark ? primary.light : primary.dark, | ||
| }, | ||
| status: { | ||
| ...base.status, | ||
| info: main, | ||
| running: main, | ||
| }, | ||
| navigation: { | ||
| ...base.navigation, | ||
| indicator: main, | ||
| selectedColor: isDark ? primary.light : main, | ||
| }, | ||
| banner: { | ||
| ...base.banner, | ||
| info: main, | ||
| link: isDark ? primary.light : main, | ||
| }, | ||
| graph: { | ||
| ...base.graph, | ||
| edge: main, | ||
| minimapViewportTint: alpha(main, 0.1), | ||
| minimapViewportTintActive: alpha(main, isDark ? 0.25 : 0.22), | ||
| minimapViewportBorder: alpha(main, isDark ? 0.5 : 0.45), | ||
| }, | ||
| ...(isDark | ||
| ? {} | ||
| : { | ||
| gradient: { | ||
| header: `linear-gradient(90deg, ${main} 0%, ${lighten( | ||
| main, | ||
| 0.25, | ||
| )} 100%)`, | ||
| cardHeader: `linear-gradient(135deg, ${main} 0%, ${lighten( | ||
| main, | ||
| 0.25, | ||
| )} 100%)`, | ||
| burst: `linear-gradient(135deg, ${main} 0%, ${lighten( | ||
| main, | ||
| 0.5, | ||
| )} 100%)`, | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Keep generated brand tokens contrast-safe.
#0d9488, used by the branding tests, is assigned directly to links and light-mode header gradients. It has only ~3.74:1 contrast against white, so this supported configuration makes normal links and white header text fail AA. Derive contrast-safe semantic variants or reject colors that cannot satisfy required foreground/background pairs, and cover the teal configuration in theme tests.
🤖 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 `@packages/design-system/src/theme/brand.ts` around lines 61 - 104, Update the
brand token generation around the theme’s text.link and light-mode gradient
values so supported brand colors such as `#0d9488` produce WCAG AA-safe contrast
for normal links and white header text. Derive appropriate semantic variants or
reject invalid colors, and add theme coverage for the teal configuration while
preserving existing dark-mode behavior.
| theme?: { | ||
| light?: { | ||
| /** | ||
| * Brand accent for the light theme (CSS color, e.g. "#0d9488"). | ||
| * Drives the primary palette, links, sidebar selection, header | ||
| * gradients, and graph accents. | ||
| * @visibility frontend | ||
| */ | ||
| primaryColor?: string; | ||
| }; | ||
| dark?: { | ||
| /** | ||
| * Brand accent for the dark theme. Dark header gradients keep the | ||
| * default navy ramp in this version. | ||
| * @visibility frontend | ||
| */ | ||
| primaryColor?: string; | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
For @material-ui/core 4.12.4, which color formats are accepted by colorManipulator lighten/darken? Specifically verify named CSS colors and CSS var() values.
💡 Result:
In @material-ui/core version 4.12.4, the colorManipulator functions lighten and darken are designed to parse and manipulate specific CSS color strings [1]. They do not support CSS var values or named CSS colors (e.g., "red", "blue") [1][2][3]. Accepted Formats The lighten and darken functions accept the following CSS color formats: - Hexadecimal colors: #nnn and #nnnnnn [1] - RGB colors: rgb and rgba [1] - HSL colors: hsl and hsla [1] Limitations - CSS var values: The colorManipulator functions cannot resolve or manipulate CSS variables because they operate on the color string at the JavaScript level [4][2]. Attempting to pass a CSS variable (e.g., var(--my-color)) will typically result in an error or failed manipulation, as the utility expects a format it can decompose into numerical components [2][3]. - Named CSS colors: Named colors are not supported [1][3]. The underlying logic requires color values to be in one of the supported functional or hex formats to perform mathematical operations [1]. For users needing to work with CSS variables in newer versions of Material UI (MUI v6+), native CSS solutions such as color-mix or the theme.lighten/theme.darken adapters (when nativeColor is enabled) are recommended, as these can handle CSS variables by leveraging browser-native CSS functionality [4][5][6]. However, these modern capabilities are not present in @material-ui/core 4.12.4 [7][5].
Citations:
- 1: https://github.com/mui/material-ui/blob/v4.12.3/packages/material-ui/src/styles/colorManipulator.js
- 2: [colorManipulator] Defining a css theme variable with rgb() and var() not working mui/material-ui#37901
- 3: [experimental_extendTheme] Unable to use custom css variable in theme mui/material-ui#41304
- 4: [colorManipulator] lighten/darken a CSS var() color mui/material-ui#43324
- 5: [material-ui] Replace JS color manipulation with native color syntax mui/material-ui#43942
- 6: https://mui.com/material-ui/customization/css-theme-variables/native-color/
- 7: https://mui.com/material-ui/migration/upgrade-to-native-color/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching config.d.ts and theme-related files:\n'
git ls-files | rg '(^|/)config\.d\.ts$|theme|brand|color' || true
printf '\nRelevant config.d.ts section:\n'
if [ -f packages/portal-app/config.d.ts ]; then
nl -ba packages/portal-app/config.d.ts | sed -n '1,90p'
fi
printf '\nSearch for primaryColor and theme creation/resolution:\n'
rg -n "primaryColor|theme\.(light|dark)|colorManipulator|getContrastText|createTheme|createMuiTheme|palette" -S packages || true
printf '\nDependency versions:\n'
for f in package.json packages/*/package.json pnpm-lock.yaml package-lock.json yarn.lock; do
[ -f "$f" ] && { printf '\n-- %s --\n'; rg -n '"`@material-ui/core`"|"`@mui/material`"|version:|`@mui/material`@|`@material-ui/core`@' "$f" | head -80 || true; }
doneRepository: openchoreo/backstage-plugins
Length of output: 1426
Validate custom primaryColor before deriving theme shades.
primaryColor is documented as any CSS color, but the brand resolution passes it through MUI v4 color manipulation, which doesn’t support named colors or CSS variables. That can crash theme construction from otherwise valid config input; validate/normalize supported hex/rgb/hsl formats during config parsing, or restrict this schema/documentation to them.
🤖 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 `@packages/portal-app/config.d.ts` around lines 34 - 51, Update the theme
configuration handling associated with light and dark primaryColor so values are
validated or normalized during config parsing before MUI v4 shade derivation;
support only formats the color manipulation accepts (hex, rgb, and hsl), or
revise the schema and documentation to explicitly restrict the accepted formats
instead of claiming any CSS color.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Invalid brand colors and malformed app.branding values now degrade to the stock look instead of crashing the render; portalBackendFeatures includes the IDP-token root router so a single backend.add is safe; restore react-router-dom peer in the app package Signed-off-by: Kavith Lokuhewage <kaviththiranga@gmail.com>
1 participant
resolveBrandTokens + ChoreoTokensProvider in the design system; frontend- visible app.branding.* schema (name, logos, per-mode primary color) wired into portal-app themes, sidebar logos, and sign-in card. No branding config = byte-identical stock look.
Part of openchoreo/openchoreo#4330
Stock portal
With branding overrides
Summary by CodeRabbit