chore: added premium tags datasources for knowing users request to add new integrations #38110
Conversation
WalkthroughThis pull request introduces a comprehensive implementation of premium data source integrations in the Appsmith client application. The changes span multiple files, adding new components, constants, and utilities to support a feature that allows users to interact with premium integrations like Salesforce and Zendesk. The implementation includes UI components, analytics tracking, feature flags, and form handling for collecting user information when they express interest in premium data sources. Changes
Assessment against linked issues
Possibly Related PRs
Suggested Labels
Suggested Reviewers
Poem
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
Integrations Pod General
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
app/client/src/pages/Editor/IntegrationEditor/DatasourceHome.tsx (1)
Line range hint
134-144: Consider addressing TypeScript TODOs.Multiple TODO comments indicate the need for proper TypeScript types. Consider addressing these to improve type safety:
// Example fix for one of the TODOs showUnsupportedPluginDialog: (callback: () => void) => void;app/client/src/pages/Editor/IntegrationEditor/RequestNewIntegration/index.tsx (1)
Line range hint
45-58: Consider enhancing analytics tracking for premium integrationsSince this component is now part of the premium integrations feature, consider updating the analytics event to distinguish between premium and regular integration requests.
onClick={() => { - AnalyticsUtil.logEvent("REQUEST_INTEGRATION_CTA"); + AnalyticsUtil.logEvent("REQUEST_INTEGRATION_CTA", { + source: "premium_integrations" + }); }}app/client/src/ce/pages/IntegrationEditor/PremiumDatasources/index.tsx (1)
61-65: Add loading state and error handling for integration iconsImages should have proper loading states and fallbacks.
Consider adding loading and error states:
<img alt={integration.name} className={"content-icon saasImage"} + onError={(e) => { + e.currentTarget.src = getAssetUrl("default-integration-icon.png"); + }} + loading="lazy" src={getAssetUrl(integration.icon)} />app/client/src/ce/pages/IntegrationEditor/PremiumDatasources/ContactForm.tsx (1)
53-65: Batch analytics eventsMultiple analytics events could be batched for better performance.
Consider implementing batch analytics:
+ const batchAnalytics = (events: Array<{ name: string; data: any }>) => { + events.forEach(({ name, data }) => { + AnalyticsUtil.logEvent(name, data); + }); + }; + const submitEvent = useCallback(() => { - AnalyticsUtil.logEvent( - props.isEnterprise - ? "SOON_NOTIFY_REQUEST" - : validRelevantEmail - ? "PREMIUM_MODAL_RELEVANT_SCHEDULE_CALL" - : "PREMIUM_MODAL_NOT_RELEVANT_SUBMIT", - { - integration_name: props.integrationName, - email: props.email, - }, - ); + const eventData = { + integration_name: props.integrationName, + email: props.email, + }; + batchAnalytics([ + { + name: props.isEnterprise + ? "SOON_NOTIFY_REQUEST" + : validRelevantEmail + ? "PREMIUM_MODAL_RELEVANT_SCHEDULE_CALL" + : "PREMIUM_MODAL_NOT_RELEVANT_SUBMIT", + data: eventData, + }, + ]); }, [props.email, props.integrationName, props.isEnterprise]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
app/client/src/ce/constants/messages.ts(1 hunks)app/client/src/ce/pages/IntegrationEditor/PremiumDatasources/ContactForm.tsx(1 hunks)app/client/src/ce/pages/IntegrationEditor/PremiumDatasources/index.tsx(1 hunks)app/client/src/ce/utils/analyticsUtilTypes.ts(2 hunks)app/client/src/ee/pages/IntegrationEditor/PremiumDatasources/index.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceTab.tsx(3 hunks)app/client/src/pages/Editor/IntegrationEditor/DatasourceHome.tsx(3 hunks)app/client/src/pages/Editor/IntegrationEditor/NewQuery.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/RequestNewIntegration/index.tsx(1 hunks)app/client/src/utils/formhelpers.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/ee/pages/IntegrationEditor/PremiumDatasources/index.tsx
👮 Files not reviewed due to content moderation or server errors (3)
- app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceTab.tsx
- app/client/src/ce/utils/analyticsUtilTypes.ts
- app/client/src/ce/constants/messages.ts
🔇 Additional comments (4)
app/client/src/pages/Editor/IntegrationEditor/NewQuery.tsx (1)
59-61: LGTM! Clean implementation for component extensibility.
The change from self-closing to regular tag enables passing child components, which aligns well with the premium datasource UI requirements.
app/client/src/pages/Editor/IntegrationEditor/DatasourceHome.tsx (2)
1-1: LGTM! Proper implementation of React children support.
The changes correctly implement children support with proper typing and rendering placement.
Also applies to: 135-135, 298-298
298-298: Verify premium datasource UI rendering.
The children are rendered at the end of the datasource grid. Ensure this placement works well with the premium datasource UI design.
✅ Verification successful
Premium datasource UI placement is correctly implemented
The children prop is rendered at the end of the DatasourceCardsContainer grid, which is the appropriate location for premium datasource UI elements. The implementation follows the established pattern where premium content is displayed alongside regular datasource cards with proper styling and grid layout.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for premium-related UI components that might be passed as children
rg -l "Premium.*?(Modal|Card|Component)" --type ts --type tsx
Length of output: 91
Script:
#!/bin/bash
# Search for premium-related UI components with correct file type flags
rg -l "Premium.*?(Modal|Card|Component)" -t ts -t tsx
# Search for any premium-related UI components in the datasource context
rg -l "Premium.*?Datasource" -t ts -t tsx
# Look for children prop usage in DatasourceHome
rg -C 3 "children" "app/client/src/pages/Editor/IntegrationEditor/DatasourceHome.tsx"
# Find any premium feature checks around datasources
rg -C 3 "isPremium.*?Datasource" -t ts -t tsx
Length of output: 694
Script:
#!/bin/bash
# Search for premium-related UI components using grep instead
grep -r "Premium.*\(Modal\|Card\|Component\)" --include="*.ts" --include="*.tsx" .
# Look at the DatasourceHome component structure
cat "app/client/src/pages/Editor/IntegrationEditor/DatasourceHome.tsx"
# Find any premium-related imports in DatasourceHome
grep -r "import.*Premium" "app/client/src/pages/Editor/IntegrationEditor/DatasourceHome.tsx"
# Check for premium feature flags or constants
grep -r "PREMIUM" --include="*.ts" --include="*.tsx" app/client/src/
Length of output: 16856
app/client/src/pages/Editor/IntegrationEditor/RequestNewIntegration/index.tsx (1)
Line range hint 24-35: Verify modal content alignment with premium features
The modal content should be verified to ensure it includes appropriate messaging for premium integrations as specified in the PR objectives (different CTAs based on email domain relevance).
app/client/src/ce/pages/IntegrationEditor/PremiumDatasources/index.tsx
Outdated
Show resolved
Hide resolved
app/client/src/ce/pages/IntegrationEditor/PremiumDatasources/ContactForm.tsx
Outdated
Show resolved
Hide resolved
app/client/src/ce/pages/IntegrationEditor/PremiumDatasources/ContactForm.tsx
Outdated
Show resolved
Hide resolved
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12277791146. |
|
Deploy-Preview-URL: https://ce-38110.dp.appsmith.com |
app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceTab.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/client/src/ce/constants/messages.ts (1)
2600-2600: Add article 'an' in the error messageIn the error message, consider changing "Please enter email" to "Please enter an email" for grammatical correctness.
Apply this diff to fix the error:
- ERROR: () => "Please enter email", + ERROR: () => "Please enter an email",app/client/src/ce/utils/PremiumDatasourcesHelpers.ts (2)
16-17: Rename variable for clarityConsider renaming
validRelevantEmailtoisRelevantEmailorisEmailRelevantto better reflect that it's a boolean value derived fromisRelevantEmail(email).Also applies to: 31-32, 50-51, 62-63
57-59: Simplify by removing unnecessary functionThe
shouldLearnMoreButtonBeVisiblefunction always returnstrue. Consider removing this function and usingtruedirectly to simplify the code.Apply this diff:
-export const shouldLearnMoreButtonBeVisible = () => { - return true; -};
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/client/src/ce/constants/PremiumDatasourcesConstants.ts(1 hunks)app/client/src/ce/constants/messages.ts(1 hunks)app/client/src/ce/pages/IntegrationEditor/PremiumDatasources/ContactForm.tsx(1 hunks)app/client/src/ce/pages/IntegrationEditor/PremiumDatasources/index.tsx(1 hunks)app/client/src/ce/utils/PremiumDatasourcesHelpers.ts(1 hunks)app/client/src/ee/constants/PremiumDatasourcesConstants.ts(1 hunks)app/client/src/ee/utils/PremiumDatasourcesHelpers.ts(1 hunks)app/client/src/utils/ProductRamps/RampsControlList.ts(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- app/client/src/ee/utils/PremiumDatasourcesHelpers.ts
- app/client/src/ee/constants/PremiumDatasourcesConstants.ts
- app/client/src/ce/constants/PremiumDatasourcesConstants.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/ce/pages/IntegrationEditor/PremiumDatasources/index.tsx
🔇 Additional comments (3)
app/client/src/ce/pages/IntegrationEditor/PremiumDatasources/ContactForm.tsx (2)
52-58: Add rate limiting to form submission
The form submission should be rate-limited to prevent abuse.
130-138: Strengthen email validation
The current email validation could be more robust.
app/client/src/utils/ProductRamps/RampsControlList.ts (1)
25-25: LGTM! The enum entry follows the naming convention.
The new PremiumDatasourcesContactModal section aligns with the PR's objective of implementing premium datasource features.
app/client/src/ce/pages/IntegrationEditor/PremiumDatasources/ContactForm.tsx
Outdated
Show resolved
Hide resolved
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12358007720. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/client/src/ce/pages/IntegrationEditor/PremiumDatasources/ContactForm.tsx (1)
53-59: 🛠️ Refactor suggestionImplement rate limiting on form submission
To prevent abuse from rapid repeated submissions, consider adding rate limiting to the form submission logic.
🧹 Nitpick comments (3)
app/client/src/ce/utils/PremiumDatasourcesHelpers.ts (3)
50-60: Simplify nested ternary operators for better readabilityConsider refactoring the nested ternary operators to improve readability and maintainability. Assign the event name to a variable before calling
logEvent.Apply this diff to refactor the function:
export const handleSubmitEvent = ( integrationName: string, email: string, isBusinessOrEnterprise?: boolean, ) => { const validRelevantEmail = isRelevantEmail(email); + let eventName = ""; + if (isBusinessOrEnterprise) { + eventName = "SOON_NOTIFY_REQUEST"; + } else if (validRelevantEmail) { + eventName = "PREMIUM_MODAL_RELEVANT_SCHEDULE_CALL"; + } else { + eventName = "PREMIUM_MODAL_NOT_RELEVANT_SUBMIT"; + } - AnalyticsUtil.logEvent( - isBusinessOrEnterprise - ? "SOON_NOTIFY_REQUEST" - : validRelevantEmail - ? "PREMIUM_MODAL_RELEVANT_SCHEDULE_CALL" - : "PREMIUM_MODAL_NOT_RELEVANT_SUBMIT", - { - integration_name: integrationName, - email, - }, - ); + AnalyticsUtil.logEvent(eventName, { + integration_name: integrationName, + email, + }); };
76-81: Refactor nested ternary operators for clarityTo enhance readability, refactor the nested ternary operators by assigning the description to a variable before returning it.
Apply this diff to refactor the function:
export const getContactFormModalDescription = ( email: string, isBusinessOrEnterprise?: boolean, ) => { const validRelevantEmail = isRelevantEmail(email); + let description = ""; + if (isBusinessOrEnterprise) { + description = createMessage(PREMIUM_DATASOURCES.COMING_SOON_DESCRIPTION); + } else if (validRelevantEmail) { + description = createMessage(PREMIUM_DATASOURCES.RELEVANT_EMAIL_DESCRIPTION); + } else { + description = createMessage(PREMIUM_DATASOURCES.NON_RELEVANT_EMAIL_DESCRIPTION); + } + return description; - return isBusinessOrEnterprise - ? createMessage(PREMIUM_DATASOURCES.COMING_SOON_DESCRIPTION) - : validRelevantEmail - ? createMessage(PREMIUM_DATASOURCES.RELEVANT_EMAIL_DESCRIPTION) - : createMessage(PREMIUM_DATASOURCES.NON_RELEVANT_EMAIL_DESCRIPTION); };
95-100: Improve readability by simplifying nested ternary operatorsRefactor the nested ternary operators to make the code more understandable. Assign the button text to a variable before returning it.
Apply this diff to refactor the function:
export const getContactFormSubmitButtonText = ( email: string, isBusinessOrEnterprise?: boolean, ) => { const validRelevantEmail = isRelevantEmail(email); + let buttonText = ""; + if (isBusinessOrEnterprise) { + buttonText = createMessage(PREMIUM_DATASOURCES.NOTIFY_ME); + } else if (validRelevantEmail) { + buttonText = createMessage(PREMIUM_DATASOURCES.SCHEDULE_CALL); + } else { + buttonText = createMessage(PREMIUM_DATASOURCES.SUBMIT); + } + return buttonText; - return isBusinessOrEnterprise - ? createMessage(PREMIUM_DATASOURCES.NOTIFY_ME) - : validRelevantEmail - ? createMessage(PREMIUM_DATASOURCES.SCHEDULE_CALL) - : createMessage(PREMIUM_DATASOURCES.SUBMIT); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/client/src/ce/constants/messages.ts(1 hunks)app/client/src/ce/pages/IntegrationEditor/PremiumDatasources/ContactForm.tsx(1 hunks)app/client/src/ce/pages/IntegrationEditor/PremiumDatasources/index.tsx(1 hunks)app/client/src/ce/utils/PremiumDatasourcesHelpers.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/ce/pages/IntegrationEditor/PremiumDatasources/index.tsx
🔇 Additional comments (3)
app/client/src/ce/pages/IntegrationEditor/PremiumDatasources/ContactForm.tsx (2)
143-151: Enhance email validation logic
Consider strengthening the email validation to provide more precise error messages and handle additional edge cases.
102-103:
Correct the aria-label on the 'Learn More' button
The aria-label should match the button's purpose to enhance accessibility. Currently, it is set to "Close" but the button's text is "Learn More".
Apply this diff to fix the aria-label:
<Button
- aria-label="Close"
+ aria-label="Learn More"
kind="secondary"
onClick={onClickLearnMore}
size="md"
>Likely invalid or redundant comment.
app/client/src/ce/constants/messages.ts (1)
2585-2608: Premium datasource messages are well-defined
The constants for premium datasources are properly structured and the messages are consistent.
app/client/src/ce/pages/IntegrationEditor/PremiumDatasources/ContactForm.tsx
Outdated
Show resolved
Hide resolved
|
Deploy-Preview-URL: https://ce-38110.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/ce/pages/IntegrationEditor/PremiumDatasources/ContactForm.tsx (1)
146-148: Consider separating email validation conditions.The validation combines existence and format checks into a single condition.
-if (!values.email || !isEmail(values.email)) { +if (!values.email) { + errors.email = createMessage(PREMIUM_DATASOURCES.FORM_EMAIL.REQUIRED); +} else if (!isEmail(values.email)) { errors.email = createMessage(PREMIUM_DATASOURCES.FORM_EMAIL.ERROR); }app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceTab.tsx (1)
398-400: Consider using optional chaining for feature flag access.The double bang (!!) could be replaced with optional chaining for better readability.
-const isPremiumDatasourcesViewEnabled = - !!featureFlags?.ab_premium_datasources_view_enabled; +const isPremiumDatasourcesViewEnabled = + featureFlags?.ab_premium_datasources_view_enabled ?? false;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/client/src/ce/entities/FeatureFlag.ts(2 hunks)app/client/src/ce/pages/IntegrationEditor/PremiumDatasources/ContactForm.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceTab.tsx(8 hunks)app/client/src/utils/ProductRamps/RampsControlList.ts(3 hunks)app/client/src/utils/formhelpers.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/src/utils/formhelpers.ts
- app/client/src/utils/ProductRamps/RampsControlList.ts
🧰 Additional context used
📓 Learnings (2)
app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceTab.tsx (1)
Learnt from: sneha122
PR: appsmithorg/appsmith#29377
File: app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceTab.tsx:316-329
Timestamp: 2024-11-12T08:11:25.416Z
Learning: The user clarified that the feature being introduced is experimental and that automated tests will be added once the decision to keep the feature is made. This context is important for future reviews as it explains the absence of tests for certain features under development.
app/client/src/ce/pages/IntegrationEditor/PremiumDatasources/ContactForm.tsx (1)
Learnt from: AmanAgarwal041
PR: appsmithorg/appsmith#38110
File: app/client/src/ce/pages/IntegrationEditor/PremiumDatasources/ContactForm.tsx:53-59
Timestamp: 2024-12-16T17:36:25.195Z
Learning: In `app/client/src/ce/pages/IntegrationEditor/PremiumDatasources/ContactForm.tsx`, the `onSubmit` function only fires an event and doesn't depend on any API call response, so rate limiting is not necessary.
🔇 Additional comments (5)
app/client/src/ce/entities/FeatureFlag.ts (1)
51-51: LGTM! Feature flag follows naming convention.
The feature flag is properly named with the ab_ prefix for A/B testing and safely disabled by default.
Also applies to: 93-93
app/client/src/ce/pages/IntegrationEditor/PremiumDatasources/ContactForm.tsx (3)
32-36: LGTM! Form styling uses design system variables.
The styled component properly uses design system spacing variables for consistency.
53-59: LGTM! Form submission is simple and effective.
Based on the previous review discussion, the submission is intentionally kept simple as it only fires an event.
153-176: LGTM! Redux form configuration is well structured.
The component is properly connected to Redux with form validation and initialization.
app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceTab.tsx (1)
175-179: LGTM! Conditional rendering is properly implemented.
The PremiumDatasources component is rendered only when both showMostPopularPlugins and isPremiumDatasourcesViewEnabled are true.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12358547224. |
|
Deploy-Preview-URL: https://ce-38110.dp.appsmith.com |
…37853-new-integrations
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/constants/PremiumDatasourcesConstants.ts (1)
20-21: Consider using an enum for form identifiersSince this is a form identifier that might be used in multiple places, consider using an enum instead of a string constant.
-export const PREMIUM_INTEGRATION_CONTACT_FORM = - "PREMIUM_INTEGRATION_CONTACT_FORM"; +export enum FormIdentifiers { + PREMIUM_INTEGRATION_CONTACT = "PREMIUM_INTEGRATION_CONTACT_FORM" +}app/client/src/pages/Editor/IntegrationEditor/PremiumDatasources/ContactForm.tsx (1)
70-76: Consider memoizing the event handlerThe
submitEventcallback could be memoized usinguseCallbackto prevent unnecessary re-renders.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/client/src/constants/PremiumDatasourcesConstants.ts(1 hunks)app/client/src/pages/Editor/IntegrationEditor/PremiumDatasources/ContactForm.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/PremiumDatasources/index.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/RequestNewIntegration/index.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/src/pages/Editor/IntegrationEditor/RequestNewIntegration/index.tsx
- app/client/src/pages/Editor/IntegrationEditor/PremiumDatasources/index.tsx
🔇 Additional comments (5)
app/client/src/constants/PremiumDatasourcesConstants.ts (2)
4-7: LGTM! Well-defined TypeScript interface
The PremiumIntegration interface is well-structured with appropriate types.
9-18: LGTM! Constants implementation follows best practices
The PREMIUM_INTEGRATIONS constant correctly implements the interface and uses the airgap helper for asset URLs.
app/client/src/pages/Editor/IntegrationEditor/PremiumDatasources/ContactForm.tsx (3)
142-150: LGTM! Robust form validation
The validation logic is well-implemented with proper type checking and error messages.
54-60:
Add error handling to form submission
The onSubmit function lacks error handling for the submission process.
- const onSubmit = () => {
+ const onSubmit = async () => {
+ try {
submitEvent();
toast.show(createMessage(PREMIUM_DATASOURCES.SUCCESS_TOAST_MESSAGE), {
kind: "success",
});
props.closeModal();
+ } catch (error) {
+ toast.show(createMessage(PREMIUM_DATASOURCES.ERROR_TOAST_MESSAGE), {
+ kind: "error",
+ });
+ }
};Likely invalid or redundant comment.
44-44: Verify business edition compatibility
Please verify that the isFreePlanInstance check correctly handles business edition access to premium features.
✅ Verification successful
Let me run one more verification to check the implementation of premium datasource handling.
Let me run one more verification to check the implementation of premium datasource handling.
Business edition compatibility is correctly handled
The implementation correctly handles business edition access to premium features through the isFreePlan selector. The code consistently uses the negation of isFreePlanInstance to determine business/enterprise status across components, and this is properly passed to helper functions that manage premium feature access.
handlePremiumDatasourceClickuses the business status for analytics and feature access- Contact form and UI elements adapt their behavior based on the plan status
- The implementation follows the established pattern for premium feature handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of isFreePlan to understand the business edition handling
rg "isFreePlan" -A 5 -B 5
Length of output: 14124
Script:
#!/bin/bash
# Search for handlePremiumDatasourceClick implementation
ast-grep --pattern 'export const handlePremiumDatasourceClick = $_'
Length of output: 77499
Script:
#!/bin/bash
# Search for utils/PremiumDatasourcesHelpers.ts implementation
rg "handlePremiumDatasourceClick" -A 5 -B 5
Length of output: 3411
…37853-new-integrations
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
app/client/src/utils/PremiumDatasourcesHelpers.ts (5)
6-20: Avoid inlining two different object returns in a ternary for better readability.
Currently, the objects differ only in color and background so you might unify or parametrize them for maintainability.
22-26: Rename functions to clarify purpose.
Currently, “getTagText” might not reveal it’s intended for premium vs soon tags. Consider “getPremiumTagText” or a more descriptive name to clarify usage.
80-85: Consider fallback integration name.
If integrationName is missing or erroneous, handle it gracefully or provide a default. This reduces the chance of displaying an empty title.
87-99: Double-check multi-branch logic.
You have multiple conditionals for business and email relevance – ensure the ordering reflects real usage. Possibly combine them for improved clarity.
106-117: Handle potential locale or translation expansions.
One day, you may want to integrate i18n, so consider storing button labels in a messages file.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/client/src/ce/constants/messages.ts(1 hunks)app/client/src/pages/Editor/IntegrationEditor/PremiumDatasources/index.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/RequestNewIntegration/form.tsx(2 hunks)app/client/src/utils/PremiumDatasourcesHelpers.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/pages/Editor/IntegrationEditor/PremiumDatasources/index.tsx
🔇 Additional comments (7)
app/client/src/utils/PremiumDatasourcesHelpers.ts (4)
28-39: Ensure analytics event has robust error handling.
If the event logging fails, it won’t be very impactful here, but you may want to handle potential failures or catch/ignore errors to keep the user experience consistent.
40-59: Replace direct window.open usage with a safer approach.
Direct usage of window.open can be deemed unsafe; you can adopt a more robust navigation utility or sanitization checks.
60-78: Validate that email domain checks and relevant data are aligned with business logic.
Since you rely on isRelevantEmail, confirm it’s consistent with other email checks to avoid confusion.
100-104: Good practice for button display logic.
Conditionally toggling the “Learn more” button is clear. Keep in mind you might unify it if similar toggles appear for other premium features.
app/client/src/pages/Editor/IntegrationEditor/RequestNewIntegration/form.tsx (2)
44-44: Marking the integration field as required is appropriate.
Ensure you also handle this in server-side validation to prevent missing integrator data.
122-122: Confirm heavier email validation if needed.
Currently, the code only checks for a standard format. If you plan to accept specific domains or more robust checks, consider isRelevantEmail or advanced validation.
app/client/src/ce/constants/messages.ts (1)
2579-2609: Keep messaging open to extension.
These Premium Datasource messages look consistent with the code style. Should you require specialized messages per integration, consider a mapping structure to reduce duplication.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12407521625. |
|
Deploy-Preview-URL: https://ce-38110.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12410886789. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/utils/PremiumDatasourcesHelpers.ts (1)
43-61: Consider extracting analytics event names as constantsThe event names are hardcoded strings. Consider extracting them into named constants to prevent typos and enable easier refactoring.
+ const ANALYTICS_EVENTS = { + SOON_NOTIFY_REQUEST: "SOON_NOTIFY_REQUEST", + PREMIUM_MODAL_RELEVANT_SCHEDULE_CALL: "PREMIUM_MODAL_RELEVANT_SCHEDULE_CALL", + PREMIUM_MODAL_NOT_RELEVANT_SUBMIT: "PREMIUM_MODAL_NOT_RELEVANT_SUBMIT" + }; export const handleSubmitEvent = ( integrationName: string, email: string, isBusinessOrEnterprise?: boolean, ) => { const validRelevantEmail = isRelevantEmail(email); AnalyticsUtil.logEvent( isBusinessOrEnterprise - ? "SOON_NOTIFY_REQUEST" + ? ANALYTICS_EVENTS.SOON_NOTIFY_REQUEST : validRelevantEmail - ? "PREMIUM_MODAL_RELEVANT_SCHEDULE_CALL" + ? ANALYTICS_EVENTS.PREMIUM_MODAL_RELEVANT_SCHEDULE_CALL - : "PREMIUM_MODAL_NOT_RELEVANT_SUBMIT", + : ANALYTICS_EVENTS.PREMIUM_MODAL_NOT_RELEVANT_SUBMIT, { integration_name: integrationName, email, }, ); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/src/ce/constants/messages.ts(1 hunks)app/client/src/pages/Editor/IntegrationEditor/PremiumDatasources/index.tsx(1 hunks)app/client/src/utils/PremiumDatasourcesHelpers.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/pages/Editor/IntegrationEditor/PremiumDatasources/index.tsx
🔇 Additional comments (3)
app/client/src/utils/PremiumDatasourcesHelpers.ts (2)
1-3: LGTM!
The imports are clean and well-organized.
40-41: Replace direct window.open with a safer navigation utility
Direct usage of window.open can be unsafe. Consider using a navigation utility that includes security checks.
app/client/src/ce/constants/messages.ts (1)
2586-2609: LGTM! Well-structured message constants
The premium datasources message constants are well-organized and follow the established pattern of using arrow functions for internationalization support.
|
Deploy-Preview-URL: https://ce-38110.dp.appsmith.com |
app/client/src/pages/Editor/IntegrationEditor/PremiumDatasources/index.tsx
Show resolved
Hide resolved
app/client/src/pages/Editor/IntegrationEditor/PremiumDatasources/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/src/utils/formhelpers.ts (1)
33-33: Add JSDoc documentation for the functionAdd JSDoc documentation to describe the function's purpose, parameters, and return value.
+/** + * Checks if the email domain is not in the list of common email providers + * @param email - The email address to validate + * @returns boolean - True if email domain is not in general domains list + */ export function isRelevantEmail(email: string) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/pages/Editor/IntegrationEditor/PremiumDatasources/index.tsx(1 hunks)app/client/src/utils/formhelpers.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/pages/Editor/IntegrationEditor/PremiumDatasources/index.tsx
🔇 Additional comments (2)
app/client/src/utils/formhelpers.ts (2)
33-56: Add input validation and externalize domain list
The implementation needs defensive programming and maintainability improvements as previously noted.
34-45: Consider adding validation for company domains
Since this is used for premium features, consider maintaining a separate list of known company domains to improve validation accuracy.
…d new integrations (#38110)
Description
This PR adds an implementation to list premium datasources. This will add
ZendeskandSalesforceas the coming soon integrations.It also adds multiple events for different interactions by the user on different sections of the UI.
This feature is behind a feature flag :
ab_premium_datasources_view_enabledFixes #37853
or
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.Datasource"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12413659557
Commit: 1b2b630
Cypress dashboard.
Tags:
@tag.DatasourceSpec:
Thu, 19 Dec 2024 14:15:58 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
PremiumDatasourceContactFormcomponent for user input on premium integrations.PremiumDatasourcescomponent to display premium integrations.Improvements
Bug Fixes