chore: Use zone as form #38550
chore: Use zone as form #38550
Conversation
WalkthroughThis pull request introduces enhancements to button and zone widget functionality across multiple files in the design system and UI builder. The changes focus on adding new properties like Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
Anvil Pod
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
app/client/src/modules/ui-builder/ui/wds/WDSButtonWidget/component/RecaptchaV2.tsx (1)
Replace
anytype withstringfor ReCAPTCHA token?.executeAsync() .then((token: string) => {
- Remove the TODO comment and eslint-disable directive as they're no longer needed
- The string type is consistent with the rest of the codebase's ReCAPTCHA implementations
🔗 Analysis chain
Line range hint
42-47: Address TODO comment and improve type safety.There's a TODO comment and an eslint disable that should be addressed.
Would you like me to help create a proper type definition for the ReCAPTCHA token to remove the eslint disable comment?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for similar ReCAPTCHA implementations that might provide type information rg -l "executeAsync|ReCAPTCHA" --type tsLength of output: 221
Script:
#!/bin/bash # Check ButtonWidget implementation echo "=== ButtonWidget/component/index.tsx ====" rg -A 5 "executeAsync|ReCAPTCHA" "app/client/src/widgets/ButtonWidget/component/index.tsx" echo -e "\n=== EvaluationsSaga.ts ====" rg -A 5 "executeAsync|ReCAPTCHA" "app/client/src/sagas/EvaluationsSaga.ts" echo -e "\n=== Full RecaptchaV2.tsx ====" cat "app/client/src/modules/ui-builder/ui/wds/WDSButtonWidget/component/RecaptchaV2.tsx"Length of output: 4114
Script:
#!/bin/bash # Check for type definitions in node_modules echo "=== Checking for RecaptchaV2 type definitions ===" fd -e d.ts -e ts "recaptcha" --full-path | grep -i "react.*recaptcha" | xargs cat # Check for any local type definitions echo -e "\n=== Checking for local type definitions ===" fd -e ts "types" --full-path | xargs grep -l "recaptcha" || trueLength of output: 421
Script:
#!/bin/bash # Check local type definitions echo "=== Local type definitions ===" cat "app/client/src/modules/ui-builder/ui/wds/WDSButtonWidget/widget/types.ts" # Check if the token is used elsewhere in the codebase with proper typing echo -e "\n=== Token usage patterns ===" rg "onRecaptchaSubmitSuccess.*token" --type tsLength of output: 1450
🧹 Nitpick comments (6)
app/client/src/modules/ui-builder/ui/wds/WDSButtonWidget/component/useRecaptcha.tsx (1)
Line range hint
21-24: Remove TODO and fix eslint disable.The eslint disable for any[] type can be replaced with proper typing.
- // TODO: Fix this the next time the file is edited - // eslint-disable-next-line @typescript-eslint/no-explicit-any - onClick?: (...args: any[]) => void; + onClick?: (onReset?: () => void) => void;app/client/src/modules/ui-builder/ui/wds/WDSButtonWidget/component/index.tsx (1)
24-25: Consider caching the disabled state.Since isDisabled depends on multiple props and context, consider memoizing it to prevent unnecessary re-renders.
+ const isDisabled = useMemo( + () => props.isDisabled || (props.disableOnInvalidForm && isFormValid === false), + [props.isDisabled, props.disableOnInvalidForm, isFormValid] + ); - const isDisabled = - props.isDisabled || (props.disableOnInvalidForm && isFormValid === false);app/client/src/modules/ui-builder/ui/wds/WDSZoneWidget/widget/context.tsx (1)
47-51: Optimize form validation logic and improve type safety.The reduce operation could be optimized and made more type-safe.
- return children.reduce((isValid: boolean, child: WidgetProps) => { - const widget = dataTree[canvasWidgets[child.widgetId].widgetName]; - - return "isValid" in widget ? widget.isValid && isValid : isValid; - }, true); + return children.every((child: WidgetProps) => { + const widget = dataTree[canvasWidgets[child.widgetId].widgetName]; + return !("isValid" in widget) || widget.isValid; + });app/client/src/modules/ui-builder/ui/wds/WDSButtonWidget/component/RecaptchaV2.tsx (1)
31-33: Simplify early return conditions.The duplicate early return conditions can be combined.
- if (isDisabled) return () => onClickProp?.(onReset); - - if (isLoading) return () => onClickProp?.(onReset); + if (isDisabled || isLoading) return () => onClickProp?.(onReset);app/client/src/modules/ui-builder/ui/wds/WDSButtonWidget/component/RecaptchaV3.tsx (1)
28-30: Consider consolidating the disabled and loading conditions.The two conditions can be combined for better maintainability.
- if (props.isDisabled) return () => onClickProp?.(onReset); - - if (props.isLoading) return () => onClickProp?.(onReset); + if (props.isDisabled || props.isLoading) return () => onClickProp?.(onReset);app/client/src/modules/ui-builder/ui/wds/WDSZoneWidget/widget/index.tsx (1)
158-164: Consider adding prop types validationThe context provider implementation is clean and well-structured. Consider adding PropTypes or TypeScript interface validation for the context provider's props.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
app/client/packages/design-system/widgets/src/components/Button/src/types.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSButtonWidget/component/RecaptchaV2.tsx(2 hunks)app/client/src/modules/ui-builder/ui/wds/WDSButtonWidget/component/RecaptchaV3.tsx(2 hunks)app/client/src/modules/ui-builder/ui/wds/WDSButtonWidget/component/index.tsx(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSButtonWidget/component/useRecaptcha.tsx(2 hunks)app/client/src/modules/ui-builder/ui/wds/WDSButtonWidget/config/defaultsConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSButtonWidget/config/propertyPaneConfig/contentConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSButtonWidget/widget/index.tsx(3 hunks)app/client/src/modules/ui-builder/ui/wds/WDSButtonWidget/widget/types.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSZoneWidget/widget/config/defaultConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSZoneWidget/widget/config/propertyPaneContent.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSZoneWidget/widget/context.tsx(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSZoneWidget/widget/index.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: client-lint / client-lint
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
- GitHub Check: chromatic-deployment
🔇 Additional comments (15)
app/client/src/modules/ui-builder/ui/wds/WDSButtonWidget/widget/types.ts (1)
12-12: LGTM! Verify event handler string evaluation.The change from function to string type for onClick aligns with widget patterns.
Also applies to: 20-20
✅ Verification successful
Verified: onClick string property is properly handled
The change is correctly implemented using the widget system's executeAction mechanism for processing onClick string actions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any existing onClick string evaluations rg -l "evaluateActionBindings.*onClick"Length of output: 41
Script:
#!/bin/bash # Search for button event handling patterns rg -l "onClick.*eval|eval.*onClick" # Look for widget evaluation utilities ast-grep --pattern 'function $_($$_) { $$$ eval($$_) $$$ }' # Check ButtonWidget files for event handling fd -e ts -e js ButtonWidget --exec rg -l "handleClick|onClick|evaluate"Length of output: 757
Script:
#!/bin/bash # Look for widget action handling patterns rg -l "executeAction|processAction|handleAction" app/client/src/modules/ui-builder # Check base widget implementations fd -e ts BaseWidget --exec rg -l "onClick|executeAction" # Search for action evaluation utilities ast-grep --pattern 'class $_ extends BaseWidget { $$$ onClick($$$) { $$$ } $$$ }'Length of output: 917
Script:
#!/bin/bash # Check WDSButtonWidget implementation rg -A 5 "executeAction" app/client/src/modules/ui-builder/ui/wds/WDSButtonWidget/widget/index.tsx # Look for action execution utility rg -A 5 "executeAction.*onClick" app/client/src/modules/ui-builder/ui/wds/WDSButtonWidget/widget/index.tsxLength of output: 419
app/client/src/modules/ui-builder/ui/wds/WDSButtonWidget/config/defaultsConfig.ts (1)
18-19: LGTM! Clear and sensible defaults.The property rename improves clarity, and both form-related features default to false.
app/client/src/modules/ui-builder/ui/wds/WDSButtonWidget/component/index.tsx (1)
30-35: LGTM! Clean Button implementation.Props spreading and event handling are implemented correctly.
app/client/packages/design-system/widgets/src/components/Button/src/types.ts (1)
44-47: LGTM! Well-documented form-related button props.The new props
disableOnInvalidFormandresetFormOnClickare clearly named and enhance the button's form functionality.app/client/src/modules/ui-builder/ui/wds/WDSZoneWidget/widget/config/defaultConfig.ts (1)
26-26: LGTM! Simple and clear addition of useAsForm property.app/client/src/modules/ui-builder/ui/wds/WDSButtonWidget/component/RecaptchaV3.tsx (2)
20-23: Props destructuring looks good with the new onReset parameter.
46-46: LGTM! Proper handling of onReset in success callback.app/client/src/modules/ui-builder/ui/wds/WDSButtonWidget/config/propertyPaneConfig/contentConfig.ts (1)
121-147: Well-structured form settings section with clear documentation.The new form-related properties are properly configured with appropriate validation and helpful descriptions.
app/client/src/modules/ui-builder/ui/wds/WDSButtonWidget/widget/index.tsx (4)
Line range hint
73-91: LGTM! Clean implementation of onButtonClick with reset handling.
98-100: Simplified hasOnClickAction implementation looks good.
142-157: Props are properly passed to ButtonComponent.All new form-related props are correctly included.
129-136: Consider adding error handling for failed form reset.The form reset operation might fail, and it would be good to handle such cases.
app/client/src/modules/ui-builder/ui/wds/WDSZoneWidget/widget/index.tsx (3)
37-37: LGTM! Clean import statement following the project's conventions.
172-172: LGTM! Clean interface extension with proper TypeScript optional boolean prop.
147-149: Verify error handling in resetChildrenMetaPropertyThe onReset implementation looks clean, but we should verify error handling in the parent method.
✅ Verification successful
Error handling verification complete - Implementation is safe
The resetChildrenMetaProperty implementation includes appropriate null checks and follows type-safe patterns through the component hierarchy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for resetChildrenMetaProperty implementation to verify error handling ast-grep --pattern 'resetChildrenMetaProperty(widgetId$_) { $$$ }'Length of output: 70
Script:
#!/bin/bash # Try broader ast-grep pattern for TypeScript method ast-grep --pattern 'resetChildrenMetaProperty($_) { $$$ }' # Fallback to ripgrep echo "=== Ripgrep search ===" rg "resetChildrenMetaProperty" -A 5Length of output: 9859
| { | ||
| propertyName: "useAsForm", | ||
| label: "Use as a form", | ||
| helpText: "Controls the visibility of the widget", | ||
| controlType: "SWITCH", | ||
| isJSConvertible: true, | ||
| isBindProperty: true, | ||
| isTriggerProperty: false, | ||
| validation: { type: ValidationTypes.BOOLEAN }, | ||
| }, |
There was a problem hiding this comment.
Fix incorrect helpText for useAsForm property.
The helpText "Controls the visibility of the widget" is incorrect for the useAsForm property. It should describe the form functionality instead.
- helpText: "Controls the visibility of the widget",
+ helpText: "Enables form functionality for this zone widget",📝 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.
| { | |
| propertyName: "useAsForm", | |
| label: "Use as a form", | |
| helpText: "Controls the visibility of the widget", | |
| controlType: "SWITCH", | |
| isJSConvertible: true, | |
| isBindProperty: true, | |
| isTriggerProperty: false, | |
| validation: { type: ValidationTypes.BOOLEAN }, | |
| }, | |
| { | |
| propertyName: "useAsForm", | |
| label: "Use as a form", | |
| helpText: "Enables form functionality for this zone widget", | |
| controlType: "SWITCH", | |
| isJSConvertible: true, | |
| isBindProperty: true, | |
| isTriggerProperty: false, | |
| validation: { type: ValidationTypes.BOOLEAN }, | |
| }, |
| }) => { | ||
| const { onReset, useAsForm, widget } = props; | ||
| const canvasWidgets = useSelector(getCanvasWidgets); | ||
| const dataTree = useSelector(getDataTree); |
There was a problem hiding this comment.
@riodeuno Need your review here. To track the invalid state of children's, I am tapping into data tree. Hope that's okay.
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 3, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/WidgetProvider/constants.ts (1)
Line range hint
1-3: Consider addressing TODO commentsMultiple TODO comments indicate pending tasks to move types to different files and fix ESLint issues. Consider addressing these in a separate cleanup PR.
Also applies to: 10-11, 49-50, 127-133, 208-210, 299-301
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/WidgetProvider/constants.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (1)
app/client/src/WidgetProvider/constants.ts (1)
128-128: Verify derived properties compatibilityThe type change from
DerivedPropertiesMaptoRecord<string, string>might affect existing widget configurations. Please ensure all widgets using derived properties are compatible with this stricter type.Run this script to find potential incompatibilities:
✅ Verification successful
Type change is safe - matches existing implementation
The change from
DerivedPropertiesMaptoRecord<string, string>is safe asDerivedPropertiesMapis already defined asRecord<string, string>in the codebase. This is just replacing a type alias with its actual definition.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for derived property usage in widget configurations ast-grep --pattern 'derived: { $$$ }' # Search for DerivedPropertiesMap type usage rg "DerivedPropertiesMap"Length of output: 14766
|
🔴🔴🔴 Cyclic Dependency Check: This PR has increased the number of cyclic dependencies by 2, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/client/src/reducers/entityReducers/types.ts (1)
1-6: LGTM! Consider adding JSDoc comments.The
SupportedLayoutstype definition is well-structured. Consider adding JSDoc comments to document the purpose and usage of each layout type.+/** + * Defines the supported layout types for the application. + * @typedef {string} SupportedLayouts + */ export type SupportedLayouts = | "DESKTOP" | "TABLET_LARGE" | "TABLET" | "MOBILE" | "FLUID";app/client/src/WidgetProvider/factory/index.tsx (1)
45-49: Consider removing the re-export to maintain a single source of truth.The re-export with TODO comment suggests this is a transitional change. To prevent confusion and maintain a clear type definition source, consider:
- Updating all imports to use the new location
- Removing the re-export once all imports are updated
Let's verify the current usage to help with the migration:
#!/bin/bash # Find all files importing DerivedPropertiesMap from the factory rg "import.*DerivedPropertiesMap.*from.*WidgetProvider/factory"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/client/src/WidgetProvider/constants.ts(1 hunks)app/client/src/WidgetProvider/factory/index.tsx(1 hunks)app/client/src/WidgetProvider/factory/types.ts(1 hunks)app/client/src/constants/WidgetConstants.tsx(1 hunks)app/client/src/reducers/entityReducers/pageListReducer.tsx(1 hunks)app/client/src/reducers/entityReducers/types.ts(1 hunks)app/client/src/utils/autocomplete/defCreatorUtils.ts(1 hunks)app/client/src/utils/autocomplete/types.ts(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/client/src/constants/WidgetConstants.tsx
- app/client/src/WidgetProvider/factory/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/WidgetProvider/constants.ts
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (6)
app/client/src/utils/autocomplete/types.ts (1)
1-1: LGTM! Good architectural decision.Moving the
ExtraDeftype definition totypes.tsimproves code organization by centralizing type definitions.Also applies to: 18-18
app/client/src/utils/autocomplete/defCreatorUtils.ts (2)
10-10: LGTM! Clean type re-export implementation.The import and re-export of
ExtraDefis implemented correctly, maintaining backward compatibility while improving code organization.Also applies to: 12-12
10-12: Verify imports across the codebase.Let's ensure all imports of
ExtraDefhave been updated to use the new location.✅ Verification successful
Re-export maintains backward compatibility
The type relocation is safe as
defCreatorUtils.tsre-exports the type, maintaining compatibility for existing imports.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining direct imports from defCreatorUtils rg "import.*ExtraDef.*from.*defCreatorUtils" # Search for new imports from types rg "import.*ExtraDef.*from.*./types"Length of output: 1282
app/client/src/reducers/entityReducers/pageListReducer.tsx (2)
Line range hint
284-286: LGTM! Clean interface definition.The
AppLayoutConfiginterface is well-defined and correctly uses theSupportedLayoutstype.
19-23: Consider tracking the TODO in an issue.The re-export pattern is a good approach for maintaining backward compatibility. However, the TODO comment should be tracked in an issue to ensure it's addressed.
Let's check for other imports that need updating:
✅ Verification successful
Create an issue to track updating SupportedLayouts imports in 6 files
• CodeScannerWidget/component/index.tsx
• CameraWidget/component/index.tsx
• layoutConversionSagas.ts
• resolveCanvasWidth.test.ts
• resolveCanvasWidth.ts
• autoToFixedLayout.ts🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all files still importing SupportedLayouts from pageListReducer rg "import.*SupportedLayouts.*from.*pageListReducer"Length of output: 890
app/client/src/WidgetProvider/factory/index.tsx (1)
48-48: Track and address technical debt items.The file contains multiple TODO comments and
anytype assertions that should be addressed:
- Re-export removal after import updates
WidgetDerivedPropertyTypetypingwidgetBuilderMaptypingmetaPropertiestyping inWidgetTypeConfigMapLet's create a tracking issue for these items:
Would you like me to create a GitHub issue to track these technical debt items?
Also applies to: 54-54, 71-71, 566-566
|
|
||
| // TODO: Fix this the next time the file is edited | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| type WidgetDerivedPropertyType = any; | ||
| export type DerivedPropertiesMap = Record<string, string>; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant type declaration.
The WidgetDerivedPropertyType is declared as any with a TODO comment. Since DerivedPropertiesMap is now properly typed as Record<string, string>, this type declaration appears to be redundant.
-// TODO: Fix this the next time the file is edited
-// eslint-disable-next-line @typescript-eslint/no-explicit-any
-type WidgetDerivedPropertyType = any;Also applies to: 54-54
Fixes appsmithorg#38525 /ok-to-test tags="@tag.Anvil" https://github.com/user-attachments/assets/891ecf61-2850-46c4-acd3-b170196e5ab7 <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12703524629> > Commit: 64616e2 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12703524629&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Anvil` > Spec: > <hr>Fri, 10 Jan 2025 05:20:43 UTC <!-- end of auto-generated comment: Cypress test results --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Added form validation controls for buttons - Introduced ability to disable buttons on invalid form - Added option to reset form on button click - New context provider for managing widget state in UI builder - **Improvements** - Enhanced reCAPTCHA handling with reset functionality - Updated button click event management - Improved widget context management - **Configuration Updates** - New form settings for button widgets - Added `useAsForm` configuration for zone widgets These updates provide more granular control over form interactions and button behaviors. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Fixes #38525
/ok-to-test tags="@tag.Anvil"
CleanShot.2025-01-08.at.17.53.37.mp4
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12703524629
Commit: 64616e2
Cypress dashboard.
Tags:
@tag.AnvilSpec:
Fri, 10 Jan 2025 05:20:43 UTC
Summary by CodeRabbit
Release Notes
New Features
Improvements
Configuration Updates
useAsFormconfiguration for zone widgetsThese updates provide more granular control over form interactions and button behaviors.