fix: edit datasource for External saas plugins #38672
Conversation
WalkthroughThe pull request introduces significant changes to the datasource rendering and management workflow in the application. The modifications focus on enhancing the flexibility of datasource configuration, introducing a new Changes
Assessment against linked issues
Possibly related PRs
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
Bug
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12787683397. |
![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
🧹 Nitpick comments (8)
app/client/src/pages/Editor/DataSourceEditor/DatasourceFormRenderer.tsx (3)
53-55: Simplify 'firstConfigProperty' assignmentThe logical OR operation is redundant since the concatenated string is always truthy. Simplify the assignment for clarity.
Apply this diff:
const firstConfigProperty = `datasourceStorages.${currentEnvironment}.` + - children[0].configProperty || children[0].configProperty; + children[0].configProperty;
195-195: Improve type safety by specifying the typeAvoid using
anyfor theelparameter. Specify a concrete type to enhance type safety.
198-198: Use optional chaining for cleaner codeSimplify the conditional check by using optional chaining.
Apply this diff:
- if (option && option.label) { + if (option?.label) {🧰 Tools
🪛 Biome (1.9.4)
[error] 198-198: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/pages/Editor/DataSourceEditor/ExternalSaasConnection.tsx (2)
27-27: Consider adding alt text for accessibility.The image should include an alt attribute for screen readers.
- <ImageWrapper src={getAssetUrl(`${ASSETS_CDN_URL}/Illustration.png`)} /> + <ImageWrapper + src={getAssetUrl(`${ASSETS_CDN_URL}/Illustration.png`)} + alt="External SaaS Connection Setup Illustration" + />
38-51: Consider extracting setup steps to constants.The setup steps could be moved to a constants file for easier maintenance and localization.
// Add to constants file export const SETUP_STEPS = [ { title: "Simplified Flow", description: "Just enter your credentials in the modal that appears." }, // ... other steps ];app/client/src/pages/Editor/IntegrationEditor/APIOrSaasPlugins.tsx (1)
302-307: Consider using localeCompare options for better sorting.While the current sorting works, consider using
localeComparewith options for more accurate alphabetical sorting.- .sort((a, b) => { - // Sort the AI plugins alphabetically - return a.name.localeCompare(b.name); - }) + .sort((a, b) => { + return a.name.localeCompare(b.name, undefined, { sensitivity: 'base' }); + })app/client/src/index.css (2)
Line range hint
193-199: Consider implementing a z-index management system.The z-index values seem arbitrary and could lead to stacking context issues. Consider using CSS custom properties (variables) to manage z-index values systematically.
+:root { + --z-index-overlay: 20; + --z-index-modal: 21; +} .bp3-overlay-zindex { - z-index: 20 !important; + z-index: var(--z-index-overlay) !important; }
Line range hint
201-217: Consolidate duplicate styles.The color picker and icon select styles share the same properties. Consider creating a shared class to reduce duplication.
+.shared-popover { + border-radius: var(--ads-v2-border-radius) !important; + box-shadow: var(--ads-v2-shadow-popovers) !important; +} -.color-picker-input, -.color-picker-input > .bp3-popover-content { - border-radius: var(--ads-v2-border-radius) !important; -} - -.color-picker-input { - box-shadow: var(--ads-v2-shadow-popovers) !important; -} +.color-picker-input, +.color-picker-input > .bp3-popover-content, +.icon-select-popover, +.icon-select-popover > .bp3-popover-content { + @extend .shared-popover; +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
app/client/src/ce/components/EnvConfigSection/index.tsx(2 hunks)app/client/src/ce/constants/ReduxActionConstants.tsx(1 hunks)app/client/src/ce/constants/messages.ts(1 hunks)app/client/src/ce/reducers/uiReducers/explorerReducer.ts(1 hunks)app/client/src/constants/AppsmithActionConstants/formConfig/ApiDatasourceFormsButtonConfig.ts(1 hunks)app/client/src/entities/Datasource/index.ts(1 hunks)app/client/src/entities/Plugin/index.ts(1 hunks)app/client/src/index.css(4 hunks)app/client/src/pages/Editor/DataSourceEditor/DBForm.tsx(2 hunks)app/client/src/pages/Editor/DataSourceEditor/DatasourceFormRenderer.tsx(1 hunks)app/client/src/pages/Editor/DataSourceEditor/DatasourceSection.tsx(2 hunks)app/client/src/pages/Editor/DataSourceEditor/ExternalSaasConnection.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/APIOrSaasPlugins.tsx(6 hunks)app/client/src/pages/common/datasourceAuth/index.tsx(6 hunks)app/client/src/reducers/entityReducers/datasourceReducer.ts(2 hunks)app/client/src/selectors/datasourceSelectors.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/pages/Editor/DataSourceEditor/DatasourceFormRenderer.tsx
[error] 198-198: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (23)
app/client/src/pages/Editor/DataSourceEditor/DatasourceSection.tsx (2)
15-15: Importing 'DatasourceFormRenderer'The new
DatasourceFormRenderercomponent is correctly imported.
60-67: Refactoring to use 'DatasourceFormRenderer'The rendering logic has been appropriately refactored to utilize
DatasourceFormRenderer, which enhances maintainability and readability.app/client/src/constants/AppsmithActionConstants/formConfig/ApiDatasourceFormsButtonConfig.ts (1)
7-7: Update action buttons for 'EXTERNAL_SAAS'The action buttons have been updated to
["CANCEL", "CONNECT_DATASOURCE"], which aligns with the new datasource connection workflow.app/client/src/ce/components/EnvConfigSection/index.tsx (1)
20-27: LGTM! Clean refactor to component-based rendering.The switch from
renderDatasourceSectiontoDatasourceFormRenderercomponent improves code maintainability while preserving the same functionality.app/client/src/selectors/datasourceSelectors.ts (1)
42-43: LGTM! Clean selector implementation.The new selector follows the established pattern and will help manage datasource loading states effectively.
app/client/src/entities/Plugin/index.ts (1)
84-84: Verify the necessity of pluginName alongside existing name property.The interface already has a
nameproperty. Let's verify if both properties are needed and document their distinct purposes.✅ Verification successful
Both
nameandpluginNameproperties serve distinct purposes and should be retained.The
nameproperty is used for UI display and filtering, whilepluginNameserves as a technical identifier for analytics, DTOs, and internal operations. This separation of concerns is a valid pattern.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of both name and pluginName to understand the distinction rg -A 2 "plugin\.(name|pluginName)"Length of output: 4936
Script:
#!/bin/bash # Check git history for the addition of pluginName git log -p -- app/client/src/entities/Plugin/index.ts | grep -A 5 "pluginName" # Search for any documentation or comments about pluginName rg -g '!*.{json,lock}' -i "pluginName.*plugin.*name"Length of output: 17729
app/client/src/pages/Editor/DataSourceEditor/DBForm.tsx (2)
20-20: LGTM!The import statement is correctly placed and follows the project's import organization pattern.
73-75: LGTM!The conditional rendering logic for external SaaS plugins is clean and follows best practices.
app/client/src/entities/Datasource/index.ts (1)
214-214: LGTM!The type definition for
providerDatais well-structured and provides good flexibility for different value types.app/client/src/ce/reducers/uiReducers/explorerReducer.ts (1)
128-128: LGTM!The cancellation handler is correctly mapped to the
setEntityUpdateSuccessfunction, maintaining consistency with other update-related handlers.app/client/src/pages/Editor/IntegrationEditor/APIOrSaasPlugins.tsx (4)
50-50: LGTM!Clean import of the loading state selector.
215-216: LGTM!Good defensive programming by preventing click actions during loading state.
228-228: LGTM!The spinner provides good visual feedback during loading states.
343-343: LGTM!The loading state is correctly composed from both props and redux state.
app/client/src/pages/common/datasourceAuth/index.tsx (3)
90-91: LGTM! Clean enum and type definition additionThe new
CONNECT_DATASOURCEbutton type is properly added to both the enum and record type.Also applies to: 101-101
Line range hint
339-346: LGTM! Proper handling of external SaaS pluginsThe condition to skip authorization redirect for
EXTERNAL_SAASplugin type is correctly implemented.
444-455: LGTM! Consistent button component implementationThe new
CONNECT_DATASOURCEbutton follows the same pattern as other button types in the component.app/client/src/reducers/entityReducers/datasourceReducer.ts (2)
129-133: LGTM! Clean action handler implementationThe
CREATE_DATASOURCE_FROM_FORM_CANCELLEDhandler properly resets the loading state.
366-373: LGTM! Consistent error handling patternThe
UPDATE_DATASOURCE_CANCELLEDhandler follows the same pattern as other cancellation handlers.app/client/src/ce/constants/ReduxActionConstants.tsx (1)
860-861: LGTM! Proper action type constant definitionsThe new action types follow the established naming convention and are placed in the correct section.
Also applies to: 866-866
app/client/src/ce/constants/messages.ts (1)
2020-2020: LGTM! Clean message constant additionThe new
CONNECT_DATASOURCE_BUTTON_TEXTconstant follows the established pattern.app/client/src/index.css (2)
Line range hint
224-236: LGTM! Well-structured error state styling.The error menu item styling follows design system guidelines and uses semantic color variables appropriately.
251-253: Verify pointer-events requirement for Paragon frame.Since this change relates to the authentication redirect flow mentioned in the PR objectives, please verify if
pointer-events: autois necessary and doesn't interfere with other overlay interactions.✅ Verification successful
Pointer-events declaration is safe to use
The
pointer-events: autoon#paragon-connect-frameis properly scoped and doesn't conflict with other overlay interactions in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other pointer-events declarations that might conflict rg -l 'pointer-events' | xargs rg 'pointer-events'Length of output: 75581
| } catch (e) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Log exceptions for better debugging
Exceptions in renderKVArray are caught but not logged, which can hinder debugging efforts. Consider logging the error.
Apply this diff:
} catch (e) {
+ log.error(e);
return;
}📝 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.
| } catch (e) { | |
| return; | |
| } | |
| } catch (e) { | |
| log.error(e); | |
| return; | |
| } |
|
Deploy-Preview-URL: https://ce-38672.dp.appsmith.com |
app/client/src/constants/AppsmithActionConstants/formConfig/ApiDatasourceFormsButtonConfig.ts
Show resolved
Hide resolved
app/client/src/pages/Editor/DataSourceEditor/ExternalSaasConnection.tsx
Outdated
Show resolved
Hide resolved
AmanAgarwal041
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
app/client/src/index.css (1)
Line range hint
224-236: Add focus outline for better accessibility.While the error states are well-defined, the focus-visible state should include an outline for keyboard navigation accessibility.
.error-menuitem:hover:not([data-disabled]), .error-menuitem:focus-visible { background-color: var(--ads-v2-color-red-50) !important; + outline: 2px solid var(--ads-v2-color-red-500) !important; + outline-offset: -2px !important; }
🧹 Nitpick comments (1)
app/client/src/ee/pages/Editor/DataSourceEditor/ExternalSaasConnection.tsx (1)
1-3: LGTM! Consider adding a brief comment explaining the EE override pattern.The re-export pattern is correct and follows the standard CE/EE split structure.
Add a brief comment explaining the purpose:
+// Re-export CE implementation for potential EE override in the future export * from "ce/pages/Editor/DataSourceEditor/ExternalSaasConnection"; import { default as CE_ExternalSaasConnection } from "ce/pages/Editor/DataSourceEditor/ExternalSaasConnection"; export default CE_ExternalSaasConnection;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/client/src/ee/pages/Editor/DataSourceEditor/ExternalSaasConnection.tsx(1 hunks)app/client/src/index.css(3 hunks)app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceTab.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/index.css(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceTab.tsx
- app/client/src/pages/Editor/IntegrationEditor/index.css
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
🔇 Additional comments (3)
app/client/src/index.css (3)
Line range hint
193-199: LGTM! Proper z-index layering maintained.The overlay backdrop opacity and z-index values are well-aligned with the existing modal and portal layers.
Line range hint
200-216: LGTM! Consistent use of design system variables.The color picker and icon select popovers maintain visual consistency with the application's design system.
Line range hint
237-246: LGTM! Clear and purposeful UI adjustments.The no-hover class and z-index adjustments serve their intended purposes effectively.
Description
In External Saas Plugin, there were couple of issues which are solved by this PR:
Fixes #38649
Fixes #38472
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, @tag.ImportExport, @tag.Workspace"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12827204806
Commit: 5685b00
Cypress dashboard.
Tags:
@tag.Datasource, @tag.ImportExport, @tag.WorkspaceSpec:
Fri, 17 Jan 2025 11:24:01 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
ExternalSaasConnectioncomponent for managing external connections.Improvements
Bug Fixes