feat: add CarbonConnect component#37280
Conversation
WalkthroughThe changes in this pull request introduce several enhancements across multiple components. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
|
/build-deploy-preview skip-tests=true |
1 similar comment
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11793850713. |
|
Deploy-Preview-URL: https://ce-37280.dp.appsmith.com |
znamenskii-ilia
force-pushed
the
feat/37186-carbon-integration
branch
from
8997ef9 to
d5f2c71
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
app/client/src/utils/formControl/formControlTypes.ts (2)
22-22: Consider maintaining alphabetical orderingThe new
CARBON_BUTTONconstant should be placed betweenARRAY_FIELDandCHECKBOXto maintain alphabetical ordering, which helps with maintainability.ARRAY_FIELD: "ARRAY_FIELD", + CARBON_BUTTON: "CARBON_BUTTON", CHECKBOX: "CHECKBOX", - CARBON_BUTTON: "CARBON_BUTTON",
Line range hint
1-24: Consider adding TypeScript type definitionsTo improve type safety, consider adding a union type for all form control types.
export type FormControlType = | "INPUT_TEXT" | "FIXED_KEY_INPUT" | "DROP_DOWN" // ... other types | "CARBON_BUTTON"; const formControlTypes: Record<FormControlType, FormControlType> = { // ... existing object }; export default formControlTypes;app/client/src/components/editorComponents/ErrorBoundry.tsx (3)
Line range hint
39-43: Consider improving error handling typesThe
componentDidCatchmethod could benefit from proper typing instead of usingany. React providesErrorandErrorInfotypes for this purpose.- componentDidCatch(error: any, errorInfo: any) { + componentDidCatch(error: Error, errorInfo: React.ErrorInfo) {
51-61: LGTM: Clean fallback implementationThe conditional rendering logic is well-implemented, maintaining backward compatibility while supporting custom fallback UI.
Consider adjusting the JSX indentation for better readability:
{this.state.hasError ? this.props.fallback || ( - <p> - Oops, Something went wrong. - <br /> - <RetryLink onClick={() => this.setState({ hasError: false })}> - Click here to retry - </RetryLink> - </p> + <p> + Oops, Something went wrong. + <br /> + <RetryLink onClick={() => this.setState({ hasError: false })}> + Click here to retry + </RetryLink> + </p> ) : this.props.children}
Line range hint
1-1: Fix filename typo: "Boundry" should be "Boundary"The filename contains a typo that should be corrected to match the component name and standard spelling.
Rename the file from
ErrorBoundry.tsxtoErrorBoundary.tsxto maintain consistency and correct spelling.app/client/src/components/formControls/BaseControl.tsx (1)
Line range hint
31-33: Consider addressing the existing TODO comments.There are multiple TODOs about fixing eslint issues related to
@typescript-eslint/no-explicit-any. These should be addressed to improve type safety.Would you like me to help create proper type definitions to replace these
anytypes?Also applies to: 51-53, 73-75
app/client/src/pages/Editor/FormControl.tsx (1)
181-184: Consider improving type safety of dsId extraction.The current dsId extraction relies on type casting and optional chaining. Consider creating a type guard to make this more type-safe.
-const dsId = - ((formValues as Action)?.datasource as any)?.id || - (formValues as Datasource)?.id; +const dsId = isAction(formValues) + ? formValues.datasource?.id + : (formValues as Datasource)?.id; +function isAction(value: Partial<Action | Datasource>): value is Partial<Action> { + return (value as Action).datasource !== undefined; +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
app/client/src/components/editorComponents/ErrorBoundry.tsx(2 hunks)app/client/src/components/formControls/BaseControl.tsx(1 hunks)app/client/src/ee/components/formControls/CarbonButton/CarbonButton.tsx(1 hunks)app/client/src/ee/components/formControls/CarbonButton/index.ts(1 hunks)app/client/src/pages/Editor/FormControl.tsx(1 hunks)app/client/src/utils/formControl/FormControlRegistry.tsx(2 hunks)app/client/src/utils/formControl/formControlTypes.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/ee/components/formControls/CarbonButton/index.ts
🔇 Additional comments (7)
app/client/src/ee/components/formControls/CarbonButton/CarbonButton.tsx (2)
3-3: Consider adding component-specific props
The interface extends ControlProps but doesn't define any button-specific properties. Consider whether this component needs additional props for button-specific behavior.
Let's check if similar controls have specific props:
#!/bin/bash
# Description: Look for similar button control implementations
# Expected: Find examples of button-specific props
rg -g "*.ts*" -A 5 "interface.*ButtonProps.*extends.*ControlProps"5-7:
Add proper TypeScript typing and implement the component
The component has the following issues:
- Missing props typing despite having a defined interface
- Returns null, making it non-functional as a form control
Apply this diff to fix the typing:
-export const CarbonButton = () => {
+export const CarbonButton: React.FC<CarbonButtonProps> = (props) => {
return null;
};Let's verify the form control registration:
app/client/src/components/editorComponents/ErrorBoundry.tsx (1)
11-11: LGTM: Well-typed fallback prop addition
The optional fallback prop follows React conventions and enhances component flexibility.
app/client/src/components/formControls/BaseControl.tsx (1)
111-111: LGTM! The datasourceId property is well-defined.
The optional string property maintains backward compatibility and follows the existing interface patterns.
Let's verify how this property is being used:
✅ Verification successful
The datasourceId property addition is safe and properly integrated
Based on the codebase analysis:
- The property is used extensively across the application, particularly in datasource management and form generation components
- It follows TypeScript best practices with proper optional typing
- The property is used consistently with the existing patterns in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of datasourceId in form controls and related components
rg -t typescript -t javascript 'datasourceId.*ControlData|createControl.*datasourceId' app/client/src/
Length of output: 139
Script:
#!/bin/bash
# Search for usage of datasourceId in form controls and related components
rg 'datasourceId.*ControlData|createControl.*datasourceId' app/client/src/
# Search for files that might use this property
rg -w 'datasourceId' app/client/src/
# Look for the ControlData interface usage
ast-grep --pattern 'interface ControlData {
$$$
}'
Length of output: 55991
Script:
#!/bin/bash
# Search for usage of datasourceId in form control creation or usage
ast-grep --pattern 'createControl($$$, { $$$, datasourceId: $_ })'
# Look for FormControlFactory usage
ast-grep --pattern 'class FormControlFactory {
$$$
createControl($$$) {
$$$
}
$$$
}'
# Search for any direct usage of ControlData with datasourceId
rg -B2 -A2 'ControlData.*datasourceId|datasourceId.*ControlData' app/client/src/
Length of output: 250
app/client/src/pages/Editor/FormControl.tsx (1)
181-184: Verify the usage of datasourceId in downstream components.
The addition of datasourceId to the config object is well-implemented, but we should ensure all consumers of FormControlFactory.createControl handle this new property correctly.
✅ Verification successful
The datasourceId addition is safely implemented and handled
The implementation in FormControlFactory properly handles the config object with all its properties, including the newly added datasourceId. The createControl method spreads all config properties into controlProps, ensuring the new datasourceId is correctly passed down to control builders.
- FormControlFactory.createControl is used in FormControl.tsx with proper type safety
- The factory implementation in utils/formControl/FormControlFactory.tsx correctly handles all config properties
- No other direct consumers of createControl need modifications as they use different control factories (PropertyControlFactory)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for createControl usage to verify handling of datasourceId
ast-grep --pattern 'FormControlFactory.createControl($_)'
Length of output: 59
Script:
#!/bin/bash
# Search for FormControlFactory class and its usage patterns
rg -A 5 "class FormControlFactory"
# Search for createControl method definition and its implementation
rg -A 10 "createControl"
# Search for any imports of FormControlFactory
rg "import.*FormControlFactory"
Length of output: 4566
app/client/src/utils/formControl/FormControlRegistry.tsx (2)
195-199: LGTM! Registration follows established patterns
The registration of the CARBON_BUTTON control follows the consistent pattern used throughout the registry.
40-41: Verify enterprise edition imports
The imports are from the enterprise edition ('ee') directory. Ensure this component is meant to be an enterprise-only feature.
✅ Verification successful
Confirmed: CarbonButton is correctly placed in enterprise edition
The component is properly placed in the enterprise edition directory and is not referenced outside of it. The implementation shows it's a dedicated enterprise feature with proper enterprise-specific path structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this is consistently placed in enterprise edition
fd -e tsx -e ts "CarbonButton" | grep -v "test" | grep "ee/"
Length of output: 128
Script:
#!/bin/bash
# Check if CarbonButton is used outside ee directory
rg "CarbonButton" --type ts --type tsx -g '!*.test.*' -g '!ee/*'
# Check if there are any other Carbon-related components in ee
fd -e tsx -e ts "Carbon" | grep "ee/"
# Check the CarbonButton implementation to verify if it's enterprise-specific
cat app/client/src/ee/components/formControls/CarbonButton/CarbonButton.tsx
Length of output: 478
## Description > [!TIP] > _Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team)._ > > _Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR._ Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.Sanity" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11795876861> > Commit: d5f2c71 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11795876861&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Tue, 12 Nov 2024 11:53:26 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Introduced an optional `fallback` prop in the ErrorBoundary component for customizable error handling. - Added a new `datasourceId` property to enhance control configuration. - Launched the `CarbonButton` component, expanding form control options. - Registered the `CARBON_BUTTON` form control type, increasing available control types. - **Enhancements** - Improved rendering logic in the FormControl component for better user experience. - Optimized performance through refined memoization in the FormControl component. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 participants
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11795876861
Commit: d5f2c71
Cypress dashboard.
Tags:
@tag.SanitySpec:
Tue, 12 Nov 2024 11:53:26 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
fallbackprop in the ErrorBoundary component for customizable error handling.datasourceIdproperty to enhance control configuration.CarbonButtoncomponent, expanding form control options.CARBON_BUTTONform control type, increasing available control types.Enhancements