Add support to configure CSS Classes via flows - #3859
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (21)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (17)
📝 WalkthroughWalkthroughAdds ChangesCSS classes property support
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
frontend/apps/console/src/features/flows/components/resource-property-panel/ClassesPropertyField.tsxESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. frontend/apps/console/src/features/flows/components/resource-property-panel/ResourceProperties.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. frontend/apps/console/src/features/flows/components/resource-property-panel/__tests__/ClassesPropertyField.test.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/packages/design/src/components/flow/adapters/RichTextAdapter.tsx (1)
160-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract repeated
id/classNamecomposition into a shared helper.This exact
id={component.id}+[cn(base), component.classes].filter(Boolean).join(' ')pattern is duplicated 6x in this file alone, and again in ImageAdapter, StackAdapter, and TextAdapter. Consider a small shared utility (e.g.combineFlowClasses(base: string, classes?: string)) in the design package to centralize this logic.♻️ Example helper
+// e.g. in a shared utils module +export const combineFlowClasses = (base: string, classes?: string): string => + [cn(base), classes].filter(Boolean).join(' ');- id={component.id} - className={[cn('Flow--richText'), component.classes].filter(Boolean).join(' ')} + id={component.id} + className={combineFlowClasses('Flow--richText', component.classes)}Also applies to: 192-193, 218-219, 238-239, 264-265, 277-278
🤖 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 `@frontend/packages/design/src/components/flow/adapters/RichTextAdapter.tsx` around lines 160 - 161, The repeated id/className composition in RichTextAdapter should be centralized into a shared helper so the same pattern can be reused across this file and the other adapters. Add a small utility in the design package (for example a helper like combineFlowClasses(base, classes)) that returns the filtered joined class string, then update RichTextAdapter to use it wherever `id={component.id}` and `cn(...)/filter(Boolean).join(' ')` are repeated, and mirror the same refactor in ImageAdapter, StackAdapter, and TextAdapter.
🤖 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
`@frontend/apps/console/src/features/flows/components/resource-property-panel/ClassesPropertyField.tsx`:
- Around line 33-53: `ClassesPropertyFieldPropsInterface.propertyValue` is too
narrow for `Element.classes`, which can be undefined. Update the
`ClassesPropertyField` props type so `propertyValue` accepts undefined, and keep
the existing `parseClasses` handling since it already normalizes missing values.
Make the change in `ClassesPropertyFieldPropsInterface` so callers can pass
`element.classes` directly without a TypeScript mismatch.
---
Nitpick comments:
In `@frontend/packages/design/src/components/flow/adapters/RichTextAdapter.tsx`:
- Around line 160-161: The repeated id/className composition in RichTextAdapter
should be centralized into a shared helper so the same pattern can be reused
across this file and the other adapters. Add a small utility in the design
package (for example a helper like combineFlowClasses(base, classes)) that
returns the filtered joined class string, then update RichTextAdapter to use it
wherever `id={component.id}` and `cn(...)/filter(Boolean).join(' ')` are
repeated, and mirror the same refactor in ImageAdapter, StackAdapter, and
TextAdapter.
🪄 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: 9dc26fc5-ca34-4023-8363-4e92c57f1cd1
📒 Files selected for processing (15)
frontend/apps/console/src/features/flows/components/resource-property-panel/ClassesPropertyField.tsxfrontend/apps/console/src/features/flows/components/resource-property-panel/ResourceProperties.tsxfrontend/apps/console/src/features/flows/components/resources/elements/adapters/input/CheckboxAdapter.tsxfrontend/apps/console/src/features/flows/components/resources/elements/adapters/input/DefaultInputAdapter.tsxfrontend/apps/console/src/features/flows/components/resources/elements/adapters/input/OTPInputAdapter.tsxfrontend/apps/console/src/features/flows/components/resources/elements/adapters/input/PhoneNumberInputAdapter.tsxfrontend/apps/console/src/features/flows/models/elements.tsfrontend/apps/console/src/features/login-flow/components/resource-property-panel/ResourceProperties.tsxfrontend/packages/design/src/components/flow/adapters/DividerAdapter.tsxfrontend/packages/design/src/components/flow/adapters/IconAdapter.tsxfrontend/packages/design/src/components/flow/adapters/ImageAdapter.tsxfrontend/packages/design/src/components/flow/adapters/RichTextAdapter.tsxfrontend/packages/design/src/components/flow/adapters/StackAdapter.tsxfrontend/packages/design/src/components/flow/adapters/TextAdapter.tsxfrontend/packages/i18n/src/locales/en-US.ts
| export interface ClassesPropertyFieldPropsInterface { | ||
| /** | ||
| * The resource associated with the property. | ||
| */ | ||
| resource: Resource; | ||
| /** | ||
| * The key of the property. | ||
| */ | ||
| propertyKey: string; | ||
| /** | ||
| * Space-separated list of CSS class names currently configured. | ||
| */ | ||
| propertyValue: string; | ||
| /** | ||
| * The event handler for the property change. | ||
| * @param propertyKey - The key of the property. | ||
| * @param newValue - The new space-separated class list. | ||
| * @param resource - The resource associated with the property. | ||
| */ | ||
| onChange: (propertyKey: string, newValue: string, resource: Resource, debounce?: boolean) => void; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
propertyValue type should accept undefined to match Element.classes.
Element.classes is classes?: string (optional), but ClassesPropertyFieldPropsInterface.propertyValue is typed as string. If the parent passes element.classes directly, TypeScript would flag the mismatch. The parseClasses function already handles undefined via value ?? '', so the runtime guard exists — only the type annotation is too narrow.
🔧 Proposed type fix
export interface ClassesPropertyFieldPropsInterface {
/**
* The key of the property.
*/
propertyKey: string;
/**
* Space-separated list of CSS class names currently configured.
*/
- propertyValue: string;
+ propertyValue?: string;📝 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.
| export interface ClassesPropertyFieldPropsInterface { | |
| /** | |
| * The resource associated with the property. | |
| */ | |
| resource: Resource; | |
| /** | |
| * The key of the property. | |
| */ | |
| propertyKey: string; | |
| /** | |
| * Space-separated list of CSS class names currently configured. | |
| */ | |
| propertyValue: string; | |
| /** | |
| * The event handler for the property change. | |
| * @param propertyKey - The key of the property. | |
| * @param newValue - The new space-separated class list. | |
| * @param resource - The resource associated with the property. | |
| */ | |
| onChange: (propertyKey: string, newValue: string, resource: Resource, debounce?: boolean) => void; | |
| } | |
| export interface ClassesPropertyFieldPropsInterface { | |
| /** | |
| * The resource associated with the property. | |
| */ | |
| resource: Resource; | |
| /** | |
| * The key of the property. | |
| */ | |
| propertyKey: string; | |
| /** | |
| * Space-separated list of CSS class names currently configured. | |
| */ | |
| propertyValue?: string; | |
| /** | |
| * The event handler for the property change. | |
| * `@param` propertyKey - The key of the property. | |
| * `@param` newValue - The new space-separated class list. | |
| * `@param` resource - The resource associated with the property. | |
| */ | |
| onChange: (propertyKey: string, newValue: string, resource: Resource, debounce?: boolean) => void; | |
| } |
🤖 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
`@frontend/apps/console/src/features/flows/components/resource-property-panel/ClassesPropertyField.tsx`
around lines 33 - 53, `ClassesPropertyFieldPropsInterface.propertyValue` is too
narrow for `Element.classes`, which can be undefined. Update the
`ClassesPropertyField` props type so `propertyValue` accepts undefined, and keep
the existing `parseClasses` handling since it already normalizes missing values.
Make the change in `ClassesPropertyFieldPropsInterface` so callers can pass
`element.classes` directly without a TypeScript mismatch.
cec60a0 to
658d6b3
Compare
658d6b3 to
f7ae6a0
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
f7ae6a0 to
0b547bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@frontend/apps/console/src/features/flows/components/resource-property-panel/__tests__/ClassesPropertyField.test.tsx`:
- Around line 49-50: The no-delete assertion in ClassesPropertyField.test.tsx is
too weak because `queryByRole('button', {name: ''})` will miss the real delete
button name from the Tooltip mock. Update the test to use the same
accessible-name query pattern as the delete-button case in the
`ClassesPropertyField` test, so it explicitly verifies that the delete control
is absent rather than just looking for an empty-name button.
🪄 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: 2f846dbd-c84a-4368-b8cd-61a6e49a10aa
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
frontend/apps/console/src/features/flows/components/resource-property-panel/ClassesPropertyField.tsxfrontend/apps/console/src/features/flows/components/resource-property-panel/ResourceProperties.tsxfrontend/apps/console/src/features/flows/components/resource-property-panel/__tests__/ClassesPropertyField.test.tsxfrontend/apps/console/src/features/flows/components/resources/elements/adapters/input/CheckboxAdapter.tsxfrontend/apps/console/src/features/flows/components/resources/elements/adapters/input/DefaultInputAdapter.tsxfrontend/apps/console/src/features/flows/components/resources/elements/adapters/input/OTPInputAdapter.tsxfrontend/apps/console/src/features/flows/components/resources/elements/adapters/input/PhoneNumberInputAdapter.tsxfrontend/apps/console/src/features/flows/components/resources/elements/adapters/input/__tests__/CheckboxAdapter.test.tsxfrontend/apps/console/src/features/flows/components/resources/elements/adapters/input/__tests__/DefaultInputAdapter.test.tsxfrontend/apps/console/src/features/flows/components/resources/elements/adapters/input/__tests__/OTPInputAdapter.test.tsxfrontend/apps/console/src/features/flows/components/resources/elements/adapters/input/__tests__/PhoneNumberInputAdapter.test.tsxfrontend/apps/console/src/features/flows/models/elements.tsfrontend/apps/console/src/features/login-flow/components/resource-property-panel/ResourceProperties.tsxfrontend/packages/design/src/components/flow/adapters/DividerAdapter.tsxfrontend/packages/design/src/components/flow/adapters/IconAdapter.tsxfrontend/packages/design/src/components/flow/adapters/ImageAdapter.tsxfrontend/packages/design/src/components/flow/adapters/RichTextAdapter.tsxfrontend/packages/design/src/components/flow/adapters/StackAdapter.tsxfrontend/packages/design/src/components/flow/adapters/TextAdapter.tsxfrontend/packages/i18n/src/locales/en-US.tspnpm-workspace.yaml
✅ Files skipped from review due to trivial changes (1)
- frontend/packages/i18n/src/locales/en-US.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- frontend/packages/design/src/components/flow/adapters/StackAdapter.tsx
- frontend/apps/console/src/features/flows/models/elements.ts
- frontend/packages/design/src/components/flow/adapters/IconAdapter.tsx
- frontend/apps/console/src/features/flows/components/resources/elements/adapters/input/PhoneNumberInputAdapter.tsx
- frontend/packages/design/src/components/flow/adapters/TextAdapter.tsx
- frontend/apps/console/src/features/flows/components/resource-property-panel/ResourceProperties.tsx
- frontend/packages/design/src/components/flow/adapters/DividerAdapter.tsx
- pnpm-workspace.yaml
- frontend/apps/console/src/features/flows/components/resources/elements/adapters/input/DefaultInputAdapter.tsx
- frontend/apps/console/src/features/login-flow/components/resource-property-panel/ResourceProperties.tsx
- frontend/apps/console/src/features/flows/components/resource-property-panel/ClassesPropertyField.tsx
- frontend/packages/design/src/components/flow/adapters/RichTextAdapter.tsx
- frontend/apps/console/src/features/flows/components/resources/elements/adapters/input/OTPInputAdapter.tsx
- frontend/packages/design/src/components/flow/adapters/ImageAdapter.tsx
- frontend/apps/console/src/features/flows/components/resources/elements/adapters/input/CheckboxAdapter.tsx
| // Only row, no delete button should be rendered. | ||
| expect(screen.queryByRole('button', {name: ''})).not.toBeInTheDocument(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Delete-button absence assertion won't catch regressions.
queryByRole('button', {name: ''}) looks for a button with an empty accessible name. The delete button's accessible name comes from the Tooltip title ('common:actions.delete' via the mock), so this query would return null even if a delete button were incorrectly rendered. Use the same query pattern as the delete test at Line 101.
💚 Proposed fix
- // Only row, no delete button should be rendered.
- expect(screen.queryByRole('button', {name: ''})).not.toBeInTheDocument();
+ // Only row, no delete button should be rendered.
+ expect(screen.queryByRole('button', {name: 'common:actions.delete'})).not.toBeInTheDocument();📝 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.
| // Only row, no delete button should be rendered. | |
| expect(screen.queryByRole('button', {name: ''})).not.toBeInTheDocument(); | |
| // Only row, no delete button should be rendered. | |
| expect(screen.queryByRole('button', {name: 'common:actions.delete'})).not.toBeInTheDocument(); |
🤖 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
`@frontend/apps/console/src/features/flows/components/resource-property-panel/__tests__/ClassesPropertyField.test.tsx`
around lines 49 - 50, The no-delete assertion in ClassesPropertyField.test.tsx
is too weak because `queryByRole('button', {name: ''})` will miss the real
delete button name from the Tooltip mock. Update the test to use the same
accessible-name query pattern as the delete-button case in the
`ClassesPropertyField` test, so it explicitly verifies that the delete control
is absent rather than just looking for an empty-name button.
0b547bb to
c33476a
Compare
Purpose
Adds the authoring-side capability in the Flow Composer/console to configure custom
CSS Classeson flow resources, complementing the existing Element ID support.Approach
ClassesPropertyField.tsxcomponent in the resource property panel for entering/managing CSS classes on a flow element.ResourceProperties.tsx(flows feature) andResourceProperties.tsx(login-flow feature).elements.tsmodel extended with aclassesfield.CheckboxAdapter,DefaultInputAdapter,OTPInputAdapter,PhoneNumberInputAdapter) updated to pass the new field through.DividerAdapter,IconAdapter,ImageAdapter,RichTextAdapter,StackAdapter,TextAdapter) updated to render/forward the configured classes.en-US.tsfor the new property field's labels/help text.Related Issues
Related PRs
id&classesinto components javascript-sdks#21idinto components ios-sdks#6idinto components android-sdks#5idinto components flutter-sdks#4Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit
Summary by CodeRabbit