feat: implement campaign theme customization#512
Conversation
- Added support for custom campaign themes using Shadcn variables, allowing users to define palette and roundness. - Introduced new components for theme editing, including a theme tab and controls for palette selection and radius adjustment. - Updated existing templates to utilize the new theme variables for consistent styling across campaign pages. - Implemented utility functions for resolving and applying campaign themes, enhancing the overall theme management system.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
5 Skipped Deployments
|
WalkthroughAdds campaign-level theming: new color utilities, a shadcn campaign theme resolver/serializer, theme editor UI, viewer/runtime injection of generated CSS variables, template updates to consume CSS-based radii, and tests for theme/radius behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Editor as EnterpriseCampaignThemeTab
participant Backend as Campaign Config
participant Viewer as Template Viewer
participant Page as Runtime Page
User->>Editor: select palette & radius
Editor->>Backend: onStylesChange(styles)
Backend->>Viewer: provide styles (design-time)
Viewer->>Viewer: resolveCampaignShadcnTheme(styles)
Viewer->>Viewer: campaignShadcnThemeToInlineStyle -> style prop
Viewer->>Viewer: render preview with inline style
Page->>Page: resolveCampaignShadcnTheme(runtime styles)
Page->>Page: campaignShadcnThemeToCssText(theme)
Page->>Page: inject <style> with CSS vars
Page->>Page: templates consume --radius and color vars
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4dc2fe3a57
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <Card | ||
| data-testid="west-referral-invitation-card" | ||
| className="relative overflow-hidden rounded-xl border-0 py-0" | ||
| className="relative overflow-hidden border-0 py-0" | ||
| > |
There was a problem hiding this comment.
The new campaign theme feature advertises roundness customization, but the invitation card still relies on the Card component’s built-in rounded-xl class because this className no longer adds a rounded-[var(--radius)] override. As a result, when a campaign sets a custom radius (e.g. 12px), the invitation card corners stay at the default size while other templates follow the custom radius, producing inconsistent UI. Consider adding a rounded-[var(--radius)] override here so the card respects the theme radius.
Useful? React with 👍 / 👎.
| <Input | ||
| disabled={disabled} | ||
| value={value} | ||
| className={inputClassName} | ||
| onChange={(e) => onValueChange(e.target.value)} | ||
| placeholder="8px" |
There was a problem hiding this comment.
The radius input persists raw text into theme styles without validating units. If a user types a numeric value like 12 (common when editing), it gets stored as --radius: 12;, which is an invalid length for border-radius: var(--radius) and causes the rounded styles to drop entirely. This only happens with unit-less inputs but is easy to trigger; normalizing to px when the input is numeric (or rejecting invalid strings) would prevent this.
Useful? React with 👍 / 👎.
![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
🤖 Fix all issues with AI agents
In
`@editor/app/`(tenant)/~/[tenant]/(r)/r/[slug]/t/[code]/_components/invitation.tsx:
- Around line 40-47: The radius value used when building CSS for campaign themes
must be validated before embedding into dangerouslySetInnerHTML to prevent CSS
injection/XSS; update resolveCampaignShadcnTheme (or the builder used by
campaignShadcnThemeToCssText) to validate the radius string against a strict CSS
length regex (allowing formats like px, rem, em, %, ch, ex, cm, mm, in, pt, pc,
vh, vw, vmin, vmax or the literal "0") and if it fails either normalize to a
safe default (e.g., "0") or strip/escape the value; ensure the check runs
wherever radius is read/constructed so only validated values are returned to
campaignShadcnThemeToCssText and then injected.
In
`@editor/app/`(workbench)/[org]/[proj]/(console)/(campaign)/campaigns/[campaign]/design/_enterprise/theme-tab.tsx:
- Around line 165-196: The code uses two different fallbacks for the theme
palette causing inconsistent behavior; unify them by introducing a single
default (e.g., const defaultPalette = "neutral") and use it wherever palette is
derived and when enabling theming: replace the current palette assignment (const
palette = styles?.palette ?? ("neutral" satisfies
CampaignThemeConfig["palette"])) and the EnableToggle onCheckedChange fallback
(palette ?? ("blue" satisfies CampaignThemeConfig["palette"])) to both reference
defaultPalette, and update any type assertions to use
CampaignThemeConfig["palette"] if needed so onStylesChange receives the same
palette that is displayed.
In `@editor/scaffolds/www-theme-config/components/navbar-logo.tsx`:
- Around line 49-55: The onChange handler for the file input currently calls
uploader(e.target.files[0], uploadType) without handling rejections and doesn't
clear the input, so add rejection handling and clear the input value after
either success or failure. Update the anonymous onChange callback (the function
referencing uploader and onLogoChange) to await or attach .then/.catch to the
uploader Promise, call onLogoChange?.(result, uploadType) on success, log or
surface the error on rejection, and in a finally block reset the file input
value (e.g., e.currentTarget.value = '') so selecting the same file again will
trigger onChange.
In `@editor/theme/shadcn/campaign-theme.ts`:
- Around line 15-22: resolvePresetTheme currently uses the "palette in palettes"
check which can match prototype properties; change it to an own-property check
(e.g., Object.prototype.hasOwnProperty.call(palettes, palette) or
Object.hasOwn(palettes, palette)) when validating the palette key before reading
palettes[palette]; update the conditional in resolvePresetTheme so only actual
keys on the palettes object are accepted and the function falls back to null for
unknown or prototype-derived keys.
In `@editor/theme/templates/enterprise/west-referral/invitation/page.tsx`:
- Around line 178-183: The Card and header container lost border-radius classes
and should use the CSS variable approach for consistency; add
rounded-[var(--radius)] to the Card component
(data-testid="west-referral-invitation-card") and add rounded-t-[var(--radius)]
to the top/header wrapper div (the div immediately after ShineBorder) so both
elements match the theming used in referrer/page.tsx and sibling templates.
🧹 Nitpick comments (2)
editor/theme/palettes/utils/index.ts (1)
41-42: Guard against non-string radius values to avoid invalid CSS.
String(value)can emit"undefined"or"[object Object]"into CSS. Consider handling numbers explicitly and providing a safe fallback for nullish values.♻️ Possible adjustment
-const stringifyREM = (value: unknown) => - typeof value === "string" ? value : String(value); +const stringifyREM = (value: unknown) => { + if (typeof value === "number") return `${value}rem`; + if (typeof value === "string") return value; + return "0"; +};editor/app/(workbench)/[org]/[proj]/(console)/(campaign)/campaigns/[campaign]/design/_enterprise/theme-tab.tsx (1)
244-266: RedundantonSelecthandler onPaletteColorChipinsideSelectItem.The
PaletteColorChiphas anonSelectcallback (lines 251-257) that duplicates theSelect.onValueChangehandler. When a user clicks the chip, both handlers may fire, potentially causing redundant state updates. TheSelectItemalready handles selection via the parentSelectcomponent.Suggested fix - remove redundant onSelect
<PaletteColorChip primary={colors.light["--primary"]} secondary={colors.light["--secondary"]} background={colors.light["--background"]} - onSelect={() => { - onStylesChange({ - palette: - key as CampaignThemeConfig["palette"], - radius: styles?.radius, - }); - }} selected={key === palette} className="size-10 rounded-sm" />
| onChange={(e) => { | ||
| if (e.target.files?.[0]) { | ||
| uploader(e.target.files[0], uploadType).then((r) => { | ||
| onLogoChange?.(r, uploadType); | ||
| }); | ||
| } | ||
| }} |
There was a problem hiding this comment.
Handle upload failures and reset the file input.
The uploader Promise can reject without a handler, and the input value isn’t cleared, so re-selecting the same file won’t trigger onChange. Consider guarding both.
🛠️ Suggested fix
- onChange={(e) => {
- if (e.target.files?.[0]) {
- uploader(e.target.files[0], uploadType).then((r) => {
- onLogoChange?.(r, uploadType);
- });
- }
- }}
+ onChange={async (e) => {
+ const file = e.currentTarget.files?.[0];
+ if (!file) return;
+ try {
+ const result = await uploader(file, uploadType);
+ onLogoChange?.(result, uploadType);
+ } catch (error) {
+ console.error("Logo upload failed", error);
+ } finally {
+ e.currentTarget.value = "";
+ }
+ }}🤖 Prompt for AI Agents
In `@editor/scaffolds/www-theme-config/components/navbar-logo.tsx` around lines 49
- 55, The onChange handler for the file input currently calls
uploader(e.target.files[0], uploadType) without handling rejections and doesn't
clear the input, so add rejection handling and clear the input value after
either success or failure. Update the anonymous onChange callback (the function
referencing uploader and onLogoChange) to await or attach .then/.catch to the
uploader Promise, call onLogoChange?.(result, uploadType) on success, log or
surface the error on rejection, and in a finally block reset the file input
value (e.g., e.currentTarget.value = '') so selecting the same file again will
trigger onChange.
| function resolvePresetTheme(palette?: string): ResolvedTheme | null { | ||
| if (!palette) return null; | ||
| if (!(palette in palettes)) return null; | ||
| const base = palettes[palette as keyof typeof palettes] as ResolvedTheme; | ||
| return { | ||
| light: { ...base.light }, | ||
| dark: { ...base.dark }, | ||
| }; |
There was a problem hiding this comment.
Use own-property checks for palette keys.
palette in palettes treats prototype keys (e.g., __proto__) as valid, which can yield empty themes instead of falling back to default. Prefer an own-property check.
🐛 Safer key check
- if (!(palette in palettes)) return null;
+ if (!Object.prototype.hasOwnProperty.call(palettes, palette)) return null;📝 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.
| function resolvePresetTheme(palette?: string): ResolvedTheme | null { | |
| if (!palette) return null; | |
| if (!(palette in palettes)) return null; | |
| const base = palettes[palette as keyof typeof palettes] as ResolvedTheme; | |
| return { | |
| light: { ...base.light }, | |
| dark: { ...base.dark }, | |
| }; | |
| function resolvePresetTheme(palette?: string): ResolvedTheme | null { | |
| if (!palette) return null; | |
| if (!Object.prototype.hasOwnProperty.call(palettes, palette)) return null; | |
| const base = palettes[palette as keyof typeof palettes] as ResolvedTheme; | |
| return { | |
| light: { ...base.light }, | |
| dark: { ...base.dark }, | |
| }; |
🤖 Prompt for AI Agents
In `@editor/theme/shadcn/campaign-theme.ts` around lines 15 - 22,
resolvePresetTheme currently uses the "palette in palettes" check which can
match prototype properties; change it to an own-property check (e.g.,
Object.prototype.hasOwnProperty.call(palettes, palette) or
Object.hasOwn(palettes, palette)) when validating the palette key before reading
palettes[palette]; update the conditional in resolvePresetTheme so only actual
keys on the palettes object are accepted and the function falls back to null for
unknown or prototype-derived keys.
| <Card | ||
| data-testid="west-referral-invitation-card" | ||
| className="relative overflow-hidden rounded-xl border-0 py-0" | ||
| className="relative overflow-hidden border-0 py-0" | ||
| > | ||
| <ShineBorder shineColor={["#A07CFE", "#FE8FB5", "#FFBE7B"]} /> | ||
| <div className="px-4 py-1.5 m-0.5 relative border border-background rounded-t-[10px] overflow-hidden flex items-center z-10"> | ||
| <div className="px-4 py-1.5 m-0.5 relative border border-background overflow-hidden flex items-center z-10"> |
There was a problem hiding this comment.
Missing border radius - inconsistent with other templates.
The border radius classes were removed (rounded-xl from Card, rounded-t-[10px] from header) but not replaced with the CSS variable-based approach used in referrer/page.tsx (rounded-[var(--radius)]). This results in no border radius on these elements, which is inconsistent with the theming approach in sibling templates.
Suggested fix for consistency
<Card
data-testid="west-referral-invitation-card"
- className="relative overflow-hidden border-0 py-0"
+ className="relative overflow-hidden rounded-[var(--radius)] border-0 py-0"
>
<ShineBorder shineColor={["#A07CFE", "#FE8FB5", "#FFBE7B"]} />
- <div className="px-4 py-1.5 m-0.5 relative border border-background overflow-hidden flex items-center z-10">
+ <div className="px-4 py-1.5 m-0.5 relative border border-background rounded-t-[var(--radius)] overflow-hidden flex items-center z-10">📝 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.
| <Card | |
| data-testid="west-referral-invitation-card" | |
| className="relative overflow-hidden rounded-xl border-0 py-0" | |
| className="relative overflow-hidden border-0 py-0" | |
| > | |
| <ShineBorder shineColor={["#A07CFE", "#FE8FB5", "#FFBE7B"]} /> | |
| <div className="px-4 py-1.5 m-0.5 relative border border-background rounded-t-[10px] overflow-hidden flex items-center z-10"> | |
| <div className="px-4 py-1.5 m-0.5 relative border border-background overflow-hidden flex items-center z-10"> | |
| <Card | |
| data-testid="west-referral-invitation-card" | |
| className="relative overflow-hidden rounded-[var(--radius)] border-0 py-0" | |
| > | |
| <ShineBorder shineColor={["#A07CFE", "#FE8FB5", "#FFBE7B"]} /> | |
| <div className="px-4 py-1.5 m-0.5 relative border border-background rounded-t-[var(--radius)] overflow-hidden flex items-center z-10"> |
🤖 Prompt for AI Agents
In `@editor/theme/templates/enterprise/west-referral/invitation/page.tsx` around
lines 178 - 183, The Card and header container lost border-radius classes and
should use the CSS variable approach for consistency; add
rounded-[var(--radius)] to the Card component
(data-testid="west-referral-invitation-card") and add rounded-t-[var(--radius)]
to the top/header wrapper div (the div immediately after ShineBorder) so both
elements match the theming used in referrer/page.tsx and sibling templates.
- Introduced strict CSS length validation for radius overrides to prevent CSS injection. - Implemented a new function to sanitize radius values, ensuring only valid lengths are accepted. - Added unit tests to verify the behavior of radius sanitization, covering valid lengths, normalization of invalid inputs, and literal zero.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@editor/theme/shadcn/campaign-theme.ts`:
- Around line 76-91: In campaignShadcnThemeToInlineStyle guard against
palette["--radius"] being undefined before assigning to style["--radius"]; when
iterating keys in palette (variable key), check that key === "--radius" and that
palette["--radius"] is not undefined (or provide a safe fallback) and only then
set style["--radius"] = palette["--radius"]; otherwise skip the assignment or
set a sensible default so style never receives undefined for the --radius
variable.
🧹 Nitpick comments (1)
editor/theme/shadcn/campaign-theme.test.ts (1)
1-25: Good test coverage for the critical security path.The tests properly validate:
- Valid CSS lengths pass through
- CSS injection attempts are sanitized to safe default
- Literal
"0"is preservedConsider extending coverage to also verify
theme?.dark["--radius"]since the implementation sets both schemes, and adding a few more valid format cases (e.g.,"0.5rem","2em",".75rem") to ensure the regex handles decimals correctly.💡 Optional: Extended test coverage
it("accepts valid CSS lengths", () => { const theme = resolveCampaignShadcnTheme({ palette: "blue", radius: "12px", }); expect(theme?.light["--radius"]).toBe("12px"); + expect(theme?.dark["--radius"]).toBe("12px"); }); + + it("accepts decimal CSS lengths", () => { + const theme = resolveCampaignShadcnTheme({ + palette: "blue", + radius: "0.5rem", + }); + expect(theme?.light["--radius"]).toBe("0.5rem"); + }); + + it("accepts leading decimal CSS lengths", () => { + const theme = resolveCampaignShadcnTheme({ + palette: "blue", + radius: ".75rem", + }); + expect(theme?.light["--radius"]).toBe(".75rem"); + });
| export function campaignShadcnThemeToInlineStyle( | ||
| theme: ResolvedTheme, | ||
| scheme: "light" | "dark" | ||
| ): CssVars { | ||
| const palette = theme[scheme]; | ||
| const style: CssVars = {}; | ||
|
|
||
| for (const key of Object.keys(palette) as Array<keyof typeof palette>) { | ||
| if (key === "--radius") { | ||
| style[key] = palette["--radius"]; | ||
| continue; | ||
| } | ||
| style[key] = stringifyHSL(palette[key]); | ||
| } | ||
| return style; | ||
| } |
There was a problem hiding this comment.
Guard against undefined --radius value.
When iterating over Object.keys(palette), if --radius key exists but its value is undefined, assigning it directly would result in style["--radius"] = undefined. This could cause unexpected behavior or type errors depending on how the style object is consumed.
🛡️ Add a guard for the radius value
for (const key of Object.keys(palette) as Array<keyof typeof palette>) {
if (key === "--radius") {
- style[key] = palette["--radius"];
+ const radius = palette["--radius"];
+ if (radius != null) style[key] = radius;
continue;
}
style[key] = stringifyHSL(palette[key]);
}🤖 Prompt for AI Agents
In `@editor/theme/shadcn/campaign-theme.ts` around lines 76 - 91, In
campaignShadcnThemeToInlineStyle guard against palette["--radius"] being
undefined before assigning to style["--radius"]; when iterating keys in palette
(variable key), check that key === "--radius" and that palette["--radius"] is
not undefined (or provide a safe fallback) and only then set style["--radius"] =
palette["--radius"]; otherwise skip the assignment or set a sensible default so
style never receives undefined for the --radius variable.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@editor/app/`(workbench)/[org]/[proj]/(console)/(campaign)/campaigns/[campaign]/design/_enterprise/theme-tab.tsx:
- Around line 250-256: The PaletteColorChip inside SelectItem is calling
onSelect which duplicates the Select's onValueChange and causes double
onStylesChange updates; remove the onSelect prop from the PaletteColorChip (or
any click handler inside SelectItem) so selection is handled only by the
Select's onValueChange and preserve onStylesChange usage there (identify
PaletteColorChip, SelectItem, Select, onSelect, onValueChange, and
onStylesChange to locate and remove the redundant handler).
🧹 Nitpick comments (2)
editor/app/(workbench)/[org]/[proj]/(console)/(campaign)/campaigns/[campaign]/design/_enterprise/theme-tab.tsx (2)
42-45: Consider importingCampaignThemeConfigfrom the shared module.This type duplicates the definition in
editor/theme/shadcn/campaign-theme.ts. Importing from the shared location ensures consistency and avoids drift if the canonical type changes.Suggested fix
-import type palettes from "@/theme/palettes"; +import type { CampaignThemeConfig } from "@/theme/shadcn/campaign-theme"; import * as _palettes from "@/theme/palettes"; import { Input } from "@/components/ui/input"; import { Checkbox } from "@/components/ui/checkbox"; import { Slider } from "@/components/ui/slider"; -type CampaignThemeConfig = { - palette?: keyof typeof palettes; - radius?: string; -};
336-339: Hardcoded locale options may limit extensibility.Currently only Korean and English are available. If this is intentional for MVP, consider adding a comment. Otherwise, these could be derived from a shared locales configuration.
| onSelect={() => { | ||
| onStylesChange({ | ||
| palette: | ||
| key as CampaignThemeConfig["palette"], | ||
| radius: styles?.radius, | ||
| }); | ||
| }} |
There was a problem hiding this comment.
Remove redundant onSelect handler to avoid double state updates.
The PaletteColorChip is rendered inside a SelectItem, which already triggers the Select's onValueChange (lines 203-212) when clicked. Having both handlers causes onStylesChange to be called twice on selection.
Suggested fix
<PaletteColorChip
primary={colors.light["--primary"]}
secondary={colors.light["--secondary"]}
background={colors.light["--background"]}
- onSelect={() => {
- onStylesChange({
- palette:
- key as CampaignThemeConfig["palette"],
- radius: styles?.radius,
- });
- }}
selected={key === palette}
className="size-10 rounded-sm"
/>📝 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.
| onSelect={() => { | |
| onStylesChange({ | |
| palette: | |
| key as CampaignThemeConfig["palette"], | |
| radius: styles?.radius, | |
| }); | |
| }} | |
| <PaletteColorChip | |
| primary={colors.light["--primary"]} | |
| secondary={colors.light["--secondary"]} | |
| background={colors.light["--background"]} | |
| selected={key === palette} | |
| className="size-10 rounded-sm" | |
| /> |
🤖 Prompt for AI Agents
In
`@editor/app/`(workbench)/[org]/[proj]/(console)/(campaign)/campaigns/[campaign]/design/_enterprise/theme-tab.tsx
around lines 250 - 256, The PaletteColorChip inside SelectItem is calling
onSelect which duplicates the Select's onValueChange and causes double
onStylesChange updates; remove the onSelect prop from the PaletteColorChip (or
any click handler inside SelectItem) so selection is handled only by the
Select's onValueChange and preserve onStylesChange usage there (identify
PaletteColorChip, SelectItem, Select, onSelect, onValueChange, and
onStylesChange to locate and remove the redundant handler).
1 participant
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.