feat: visual Style editor with live preview#72
Conversation
A dedicated Style mode (Fields | Style toggle) for theming the public form, gated behind a "Custom styles" setting. - feat: theme model + `resolveThemeVars` (single source) → `--sfb-*` variables shared by the admin live preview and the public embed/page via one stylesheet (`form-css.ts`), so there is no duplicated styling logic. - feat: Style mode sidebar — accent / font / card + page background, field corners / style / label weight, button style / width, form width / spacing / card shadow / card border / card corners / placement (top | center), with a Desktop | Mobile live preview. - feat: editable, persisted colour palettes (add via picker, remove per swatch) per colour group; "Reset to default theme" restores them. - feat: enabling "Custom styles" in Settings reveals the Style tab immediately (no save required) with a link into the editor. - feat: style changes follow the existing draft / publish flow (theme lives in settings, snapshotted on publish); toasts announce the state on save/publish. - fix: public page applies theme vars via a <style> block instead of an inline attribute — a quoted font value no longer breaks the style and falls back to the browser's serif default. - test: theme resolver coverage (colors, corners, border, placement, palettes).
📝 WalkthroughWalkthroughAdds typed theme resolution, an admin style editor with live preview, style-mode integration in the form builder, and theme application to generated public and embedded forms. ChangesForm styling
Estimated code review effort: 4 (Complex) | ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 3
🧹 Nitpick comments (2)
tests/theme.test.ts (1)
38-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for solid button text contrast on a light accent.
The current outline-button test verifies the transparent background and accent border, but no test covers
--sfb-btn-colorfor solid buttons. A test with a light accent (e.g.#f5f5f9) would catch the bug where solid button text is always#fffregardless of accent luminance.🧪 Suggested test
it('solid button uses dark text on a light accent', () => { const v = resolveThemeVars({ ...DEFAULT_THEME, buttonStyle: 'solid', accent: '`#f5f5f9`' }); expect(v['--sfb-btn-color']).toBe('`#1f1f33`'); }); it('solid button uses white text on a dark accent', () => { const v = resolveThemeVars({ ...DEFAULT_THEME, buttonStyle: 'solid', accent: '`#1e1e2e`' }); expect(v['--sfb-btn-color']).toBe('`#fff`'); });🤖 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 `@tests/theme.test.ts` around lines 38 - 42, Add solid-button contrast coverage alongside the existing outline-button test in theme.test.ts: add cases using light and dark accents that assert resolveThemeVars returns `#1f1f33` and `#fff` respectively for --sfb-btn-color, ensuring buttonStyle is set to solid.admin/src/theme.ts (1)
115-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
--sfb-accent-contrastis computed but never consumed.This variable is resolved and returned but not referenced in
FORM_CSSor any inline style in the codebase. Either wire it into the button color (see adjacent comment) or remove it to avoid confusion.🤖 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 `@admin/src/theme.ts` at line 115, The computed --sfb-accent-contrast variable is unused. In the theme generation logic, either reference it from FORM_CSS or the relevant inline button styles as the button foreground color, or remove its definition and related computation if no consumer is needed.
🤖 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 `@admin/src/pages/FormBuilderPage.tsx`:
- Line 191: Remove the onToggleStyle prop and callback usage from
SettingsDrawer, including its invocation in the “Custom styles” ToggleRow. Keep
the toggle updating only the drawer’s local settings via patch({ enableStyle: v
}); ensure the parent’s Style tab visibility and enableStyle state continue
deriving from settings and changing only through onSave, so Cancel discards the
change.
In `@admin/src/theme.ts`:
- Line 134: Fix the solid-button text color in the theme mapping by removing the
dead `isDark(t.accent)` branches and using the existing `--sfb-accent-contrast`
value (or equivalent contrast-aware dark fallback) for light accents while
retaining white text for dark accents. Update the `--sfb-btn-color` assignment
in the theme generation logic.
In `@server/src/controllers/form.ts`:
- Line 60: Escape form.settings.theme.fieldStyle with the existing escapeHtml
helper before interpolating it into the data-sfb-fields attribute, preserving
the current conditional rendering and preventing malformed markup from persisted
values.
---
Nitpick comments:
In `@admin/src/theme.ts`:
- Line 115: The computed --sfb-accent-contrast variable is unused. In the theme
generation logic, either reference it from FORM_CSS or the relevant inline
button styles as the button foreground color, or remove its definition and
related computation if no consumer is needed.
In `@tests/theme.test.ts`:
- Around line 38-42: Add solid-button contrast coverage alongside the existing
outline-button test in theme.test.ts: add cases using light and dark accents
that assert resolveThemeVars returns `#1f1f33` and `#fff` respectively for
--sfb-btn-color, ensuring buttonStyle is set to solid.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5555ae7e-8eef-44c1-8690-07b7a84f097d
📒 Files selected for processing (7)
admin/src/components/StyleMode.tsxadmin/src/form-css.tsadmin/src/pages/FormBuilderPage.tsxadmin/src/theme.tsadmin/src/types.tsserver/src/controllers/form.tstests/theme.test.ts
|
|
||
| <div style={{ padding: '20px 0', borderBottom: `1px solid ${C.n150}` }}> | ||
| <div style={dGroupT}>Style</div> | ||
| <ToggleRow label="Custom styles" hint="Enable the Style editor to theme the public form." on={!!settings.enableStyle} onClick={() => { const v = !settings.enableStyle; patch({ enableStyle: v }); onToggleStyle(v); }} /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Style toggle bypasses the save/cancel buffer.
The "Custom styles" toggle calls both patch({ enableStyle: v }) (local buffer) and onToggleStyle(v) (parent state immediately). Every other setting in the drawer stays in the local buffer and only reaches the form on "Save settings" (per the comment on line 104). If a user toggles styles on then clicks Cancel, the parent's enableStyle is already true and the Style tab persists — the toggle is not reverted.
onToggleStyle should not mutate the parent state directly. Instead, the Style tab visibility should be derived from the local settings state inside the drawer, and the actual enableStyle change should only flow through onSave.
🐛 Proposed fix
Remove onToggleStyle from the SettingsDrawer props and the immediate parent mutation. The Style tab visibility in the parent already derives from settings.enableStyle, which only changes when the user clicks "Save settings".
function SettingsDrawer({ initialDescription, initialSettings, slug, publishedAt, onClose, onSave, onOpenStyle }: {
initialDescription: string;
initialSettings: FormSettings;
slug: string | null;
publishedAt: string | null;
onClose: () => void;
onSave: (description: string, settings: FormSettings) => void;
- onToggleStyle: (v: boolean) => void;
onOpenStyle: () => void;
}) {- <ToggleRow label="Custom styles" hint="Enable the Style editor to theme the public form." on={!!settings.enableStyle} onClick={() => { const v = !settings.enableStyle; patch({ enableStyle: v }); onToggleStyle(v); }} />
+ <ToggleRow label="Custom styles" hint="Enable the Style editor to theme the public form." on={!!settings.enableStyle} onClick={() => patch({ enableStyle: !settings.enableStyle })} /> <SettingsDrawer
initialDescription={description}
initialSettings={settings}
slug={slug}
publishedAt={publishedAt}
onClose={() => setShowSettings(false)}
onSave={(desc, s) => { setDescription(desc); setSettings(s); setShowSettings(false); }}
- onToggleStyle={(v) => setSettings((s) => ({ ...s, enableStyle: v }))}
onOpenStyle={() => { setShowSettings(false); setMode('style'); }}
/>Also applies to: 526-527
🤖 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 `@admin/src/pages/FormBuilderPage.tsx` at line 191, Remove the onToggleStyle
prop and callback usage from SettingsDrawer, including its invocation in the
“Custom styles” ToggleRow. Keep the toggle updating only the drawer’s local
settings via patch({ enableStyle: v }); ensure the parent’s Style tab visibility
and enableStyle state continue deriving from settings and changing only through
onSave, so Cancel discards the change.
| '--sfb-btn-width': t.buttonWidth === 'full' ? '100%' : 'auto', | ||
| // button: solid = accent bg / contrast text; outline = transparent bg / accent text + border | ||
| '--sfb-btn-bg': t.buttonStyle === 'solid' ? t.accent : 'transparent', | ||
| '--sfb-btn-color': t.buttonStyle === 'solid' ? (isDark(t.accent) ? '#fff' : '#fff') : t.accent, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Solid button text is always white regardless of accent luminance.
isDark(t.accent) ? '#fff' : '#fff' — both branches return '#fff', making the isDark check dead logic. For light accent colors (e.g. a user-added #f5f5f9), white button text will have poor contrast. The --sfb-accent-contrast variable (line 115) was likely intended to be used here, or the else branch should return a dark color.
🐛 Proposed fix
- '--sfb-btn-color': t.buttonStyle === 'solid' ? (isDark(t.accent) ? '`#fff`' : '`#fff`') : t.accent,
+ '--sfb-btn-color': t.buttonStyle === 'solid' ? (isDark(t.accent) ? '`#fff`' : '`#1f1f33`') : t.accent,📝 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.
| '--sfb-btn-color': t.buttonStyle === 'solid' ? (isDark(t.accent) ? '#fff' : '#fff') : t.accent, | |
| '--sfb-btn-color': t.buttonStyle === 'solid' ? (isDark(t.accent) ? '`#fff`' : '`#1f1f33`') : t.accent, |
🤖 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 `@admin/src/theme.ts` at line 134, Fix the solid-button text color in the theme
mapping by removing the dead `isDark(t.accent)` branches and using the existing
`--sfb-accent-contrast` value (or equivalent contrast-aware dark fallback) for
light accents while retaining white text for dark accents. Update the
`--sfb-btn-color` assignment in the theme generation logic.
| </head> | ||
| <body> | ||
| <div class="card"> | ||
| <div class="card"${form.settings && form.settings.theme && form.settings.theme.fieldStyle ? ` data-sfb-fields="${form.settings.theme.fieldStyle}"` : ''}> |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
fieldStyle value is not escaped in the data-sfb-fields attribute.
form.settings.theme.fieldStyle is interpolated directly into the attribute value without escapeHtml. While the TypeScript type constrains it to 'outline' | 'filled' | 'underline', this is server-side JavaScript with any-typed data from the database. A tampered persisted value containing " would break the attribute markup. The escapeHtml function is available in the same file.
🛡️ Proposed fix
- <div class="card"${form.settings && form.settings.theme && form.settings.theme.fieldStyle ? ` data-sfb-fields="${form.settings.theme.fieldStyle}"` : ''}>
+ <div class="card"${form.settings && form.settings.theme && form.settings.theme.fieldStyle ? ` data-sfb-fields="${escapeHtml(form.settings.theme.fieldStyle)}"` : ''}>📝 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.
| <div class="card"${form.settings && form.settings.theme && form.settings.theme.fieldStyle ? ` data-sfb-fields="${form.settings.theme.fieldStyle}"` : ''}> | |
| <div class="card"${form.settings && form.settings.theme && form.settings.theme.fieldStyle ? ` data-sfb-fields="${escapeHtml(form.settings.theme.fieldStyle)}"` : ''}> |
🤖 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 `@server/src/controllers/form.ts` at line 60, Escape
form.settings.theme.fieldStyle with the existing escapeHtml helper before
interpolating it into the data-sfb-fields attribute, preserving the current
conditional rendering and preventing malformed markup from persisted values.
1 participant
Summary
A dedicated Style mode for theming the public form, with a live Desktop/Mobile preview. Gated behind a "Custom styles" setting.
Architecture (single source of truth)
theme.ts— the theme model +resolveThemeVars()→ a small set of--sfb-*CSS variables (plus adata-sfb-fieldsattribute for field-style variants).form-css.ts— the form's stylesheet, imported by both the public embed (server) and the admin live preview. One stylesheet drives both surfaces; no duplicated styling logic.Controls
Accent · font · card & page background · field corners / style / label weight · button style / width · form width / spacing / card shadow / card border / card corners / placement (top | center).
Fix included
The public page now applies theme variables via a
<style>block instead of an inlinestyleattribute — a quoted font value ("Inter") no longer breaks the attribute and falls back to the browser's serif default.Verification
verify, embed syntax.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests