feat: datasource homepage ui redesign and search functionality for the datasources #38360
Conversation
WalkthroughThis pull request introduces comprehensive updates to the datasource integration interface in the Appsmith client application. The changes focus on restructuring the user interface for creating and managing datasources, introducing new components like 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
AmanAgarwal041
changed the title
AmanAgarwal041
changed the title
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12490435517. |
![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 (13)
app/client/src/pages/Editor/IntegrationEditor/IntegrationStyledComponents.tsx (2)
4-7: Maintain consistency with theme variables
Consider ensuring margin and border colors align with the design system.
28-48: Hover effect is user friendly
Hiding.ctaby default is acceptable, though some users may be unaware until they hover. Consider a subtle pointer or tooltip for guidance.app/client/src/pages/Editor/IntegrationEditor/EmptySearchedPlugins.tsx (1)
27-32: Search logic uses string comparison
toLocaleLowerCase()is acceptable for basic string search but may not handle all locales. For simple needs, this is fine.app/client/src/pages/Editor/IntegrationEditor/PremiumDatasources/index.tsx (1)
73-73: Tag usage clarifies plan type
The color scheme could be handled by theme variables, but this is acceptable.app/client/src/pages/Editor/IntegrationEditor/AIPlugins.tsx (2)
36-66: Refactor logic for clarity and reuse.
TheAIDataSourcesfunction is well-structured for listing plugins, but extracting the click handler logic into a separate helper might improve testability and readability.
86-106: Combine sorting & filtering to minimize passes onplugins.
Currently, you sort, then filter. Combining these steps or using a single pass approach can slightly enhance performance for large plugin arrays.app/client/src/pages/Editor/IntegrationEditor/MockDataSources.tsx (4)
12-19: Retain or remove named exports consistently.
Presently, styled components are imported and used here, but the naming conventions in the new imports are inconsistent with older file patterns. Revisit to maintain a uniform style.
Line range hint
27-79: Ensure consistent naming for analytics events.
You’re loggingADD_MOCK_DATASOURCE_CLICKandCREATE_DATA_SOURCE_CLICK. Use a consistent naming format prefix to group these logically.
71-79: Provide a loading or success state foraddMockDataSource.
A loader or success toast helps signal the outcome of asynchronously adding a mock datasource.
81-123: Consider migrating search logic to a memoized selector.
The filtered mock datasources rely onsearchedPlugin. Using a memoized selector or a separate state for searching can reduce re-renders when selecting or editing unrelated states.app/client/src/ce/constants/messages.ts (1)
398-410: Minimal duplication check for the “NEW_DATASOURCE_*” constants.
While adding new message constants, ensure minimal duplication or overlap with existing constants to avoid confusion and reduce maintenance overhead.app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx (2)
69-69: TODO reminder in interface.
The interface includes a// TODO. Consider addressing or opening a dedicated issue if this fix is still pending.Would you like help creating a tracking issue?
299-305: Double-negation usage.
Lines 311 and 318 use!!to coerce values to boolean, which is not strictly necessary in a conditional. Removing the extra negation won't affect logic.- const filteredMostPopularPlugins: Plugin[] = !!isAirgappedInstance + const filteredMostPopularPlugins: Plugin[] = isAirgappedInstance ? mostPopularPlugins.filter( (plugin: Plugin) => plugin?.packageName !== PluginPackageName.GOOGLE_SHEETS, ) : mostPopularPlugins; - let plugins = !!props?.showMostPopularPlugins + let plugins = props?.showMostPopularPlugins ? filteredMostPopularPlugins : getDBPlugins(state);Also applies to: 309-311, 318-320, 322-325
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
app/client/src/ce/constants/messages.ts(1 hunks)app/client/src/constants/PremiumDatasourcesConstants.ts(1 hunks)app/client/src/pages/Editor/IntegrationEditor/AIDataSources.tsx(0 hunks)app/client/src/pages/Editor/IntegrationEditor/AIPlugins.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/APIOrSaasPlugins.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/AddDatasourceSecurely.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceHeader.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceTab.tsx(5 hunks)app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx(8 hunks)app/client/src/pages/Editor/IntegrationEditor/DatasourceItem.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/EmptySearchedPlugins.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/IntegrationStyledComponents.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/MockDataSources.tsx(2 hunks)app/client/src/pages/Editor/IntegrationEditor/NewApi.tsx(0 hunks)app/client/src/pages/Editor/IntegrationEditor/NewQuery.tsx(0 hunks)app/client/src/pages/Editor/IntegrationEditor/PremiumDatasources/index.tsx(3 hunks)
💤 Files with no reviewable changes (3)
- app/client/src/pages/Editor/IntegrationEditor/AIDataSources.tsx
- app/client/src/pages/Editor/IntegrationEditor/NewQuery.tsx
- app/client/src/pages/Editor/IntegrationEditor/NewApi.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx
[error] 220-220: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
[error] 311-311: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
[error] 318-318: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (71)
app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceTab.tsx (14)
2-2: Import statement is appropriate.
No issues noted here.
21-23: Nicely structured imports for modularity.
Including APIOrSaasPlugins, DBPluginsOrMostPopular, and AIPlugins together clarifies their roles.
32-34: Well-organized imports.
These imports (StyledDivider, CreateNewDatasourceHeader, and EmptySearchedPlugins) are in line with the new layout approach.
41-41: Margin update aligns UI spacing to new design.
This minor UI update should unify styling for a consistent experience.
95-95: Simple conditional assignment.
Setting mockDataSectionVisible based on non-empty mockDatasources is straightforward.
100-108: Separate components for header and mock datasources.
Displaying AddDatasourceSecurely and optionally MockDataSources improves clarity by grouping related UI conditions.
109-109: Use of DBPluginsOrMostPopular.
Good approach for grouping database plugins or popular ones in a self-contained component.
118-122: Conditional properties and feature flags.
Passing in isPremiumDatasourcesViewEnabled helps keep premium logic in sync with feature flags.
127-129: Additional divider usage.
The addDivider prop improves readability by segmenting plugin groups.
135-141: showSaasAPIs prop usage.
This provides a clean toggle to differentiate between REST/GraphQL and SaaS display.
144-144: AI plugins integration.
Adding AIPlugins seamlessly extends the available data source options for advanced scenarios.
149-153: Conditional block for mock data.
Ensuring MockDataSources is shown only if real datasources exist. This new logic usage is consistent with previous condition.
155-159: Render fallback for empty search results.
EmptySearchedPlugins clarifies no matches for certain queries, improving UX.
183-184: Feature flag check for premium datasources.
The double-bang ensures a valid boolean, cleanly gating premium features.
app/client/src/pages/Editor/IntegrationEditor/APIOrSaasPlugins.tsx (7)
1-88: Imports and constants well-structured.
Bringing in relevant hooks, utilities, and constants such as API_ACTION ensures a clear approach for plugin creation.
89-220: NewApiScreen encapsulation looks good.
• Provides modular handling of API creation, with robust analytics logs.
• The handleOnClick logic uses a fallback for unsupported plugins, which is a helpful user safeguard.
222-240: Scroll-into-view logic for improved navigation.
This approach ensures a smooth user experience, especially when toggling between sections.
241-249: Cleanly handles airgapped instances.
Returning null early if showSaasAPIs is set in an airgapped environment is a straightforward safeguard.
251-264: Section heading and optional SaaS.
Using CREATE_NEW_SAAS_SECTION_HEADER or CREATE_NEW_API_SECTION_HEADER is a tidy design to differentiate contexts.
266-319: Selective plugin filtering and search alignment.
Multiple filter criteria, including searchedPlugin, plugin type, premium plugin array, and feature flags are combined effectively.
321-330: MapDispatchToProps usage is consistent.
All necessary actions are neatly wired, complying with the Redux pattern.
app/client/src/constants/PremiumDatasourcesConstants.ts (1)
4-4: Exporting PremiumIntegration broadens reusability.
This interface can now be leveraged across multiple components and modules.
app/client/src/pages/Editor/IntegrationEditor/DatasourceItem.tsx (2)
1-17: Interfaces and imports appear coherent.
Encapsulating details (className, description) clarifies the coin of exchange for data source items.
19-44: DatasourceItem is a concise, self-contained component.
• Highlights: Click handler, optional sibling content, and consistent naming.
• This fosters an extensible approach to data source card UI.
app/client/src/pages/Editor/IntegrationEditor/AddDatasourceSecurely.tsx (6)
3-3: Text component import is aligned with usage.
Ensures consistent typography for user-facing copy.
7-9: Callout styling approach.
Leveraging StyledCallout and a close button for dismissible UI is neat and user-friendly.
10-10: Reference to close button constants.
Good use of pre-defined class names ensures consistent styling and behavior.
12-14: Wrapper with dynamic isClosed prop.
Smart approach to conditionally hide or display the callout container.
21-21: Local state for callout visibility.
Maintaining minimal state avoids complexity for a small UI element.
24-42: AddDatasourceSecurely emphasis on encryption and trust.
• Simple, dismissible info callout that fosters user confidence.
• Coupled with a close button for better user control.
app/client/src/pages/Editor/IntegrationEditor/IntegrationStyledComponents.tsx (8)
1-2: Imports look straightforward
No issues found here.
9-13: General styling approach
Flex column with defined gap is clear. Good implementation.
15-17: Logical font sizing
Using theme-based font sizing is aligned to consistency.
19-26: Responsive grid usage
Implementation is concise and aligns items well. Looks good.
50-55: Adaptive image sizing
Good approach to prevent overflow issues.
57-62: Sensible layout for text blocks
This is a neat approach.
64-69: Clear emphasis for datasource name
No issues found.
71-76: Muted styling is correct
Implementation matches typical subdued styling for secondary text.
app/client/src/pages/Editor/IntegrationEditor/EmptySearchedPlugins.tsx (8)
1-2: Imports are well-organized
No concerns here.
3-10: Utilizing ee/constants/messages
Good use of localized strings for user-facing text.
11-15: Selectors are properly imported
Logic is consistent with Redux usage.
17-20: Styled image is well-defined
Image dimension constraints are good for maintaining layout integrity.
22-26: Prop definitions are clear
Straightforward destructuring for isPremiumDatasourcesViewEnabled.
34-42: Conditional merges multiple checks
Combining matches is efficient enough for small data sets.
44-45: Returning null early is good
This approach keeps the component clean if search results exist.
46-58: Empty search UI is user friendly
Clearly communicates no matching plugins found.
app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceHeader.tsx (7)
1-2: Importing from @appsmith/ads
Consistent usage of design system components.
15-19: Form name is well declared
This name is descriptive and unique for the Redux form.
21-24: Header text container
Flex usage is standard for layout. LGTM.
26-32: Search container styling
Height and font size are consistent with the rest of the UI.
34-36: Interface definition is minimal
Serves well for typed props.
38-60: Header structure is crisp
The heading and subheading usage is straightforward. Search is integrated neatly.
62-71: Redux connection looks good
Mapping form value to props and usage of enableReinitialize is correct.
app/client/src/pages/Editor/IntegrationEditor/PremiumDatasources/index.tsx (6)
3-3: Accurate imports
No issues here.
12-13: DatasourceItem usage recognized
Appropriate replacement for older components.
38-40: Props for flexible usage
Passing in a plugin list fosters reusability.
56-57: Mapping integration items
Good approach for dynamic rendering of premium integrations.
59-59: Providing a test-friendly className
This is helpful for test automation.
62-65: Icon and name are set properly
Leverages asset URL function effectively.
app/client/src/pages/Editor/IntegrationEditor/AIPlugins.tsx (2)
39-49: Add error handling.
When creating temporary data sources, consider including a try/catch around props.createTempDatasourceFromForm to gracefully handle any potential errors from the action.
68-84: Verify null-return conditions.
Returning null if the instance is airgapped or plugins array is empty is sensible, but ensure no up-line or downstream component depends on these states for layout or data logic.
app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx (10)
Line range hint 1-23: Imports look consistent and concise.
No issues found with these imports; they appear necessary and relevant.
30-31: Descriptive constants for heading titles look good.
The message constants are properly named and enhance readability.
36-47: Integration Styled Components and helper imports are well-structured.
This grouping of imports for styled components and utility functions is clear, keeping relevant modules together.
112-126: New props interface extends flexibility.
The expanded prop list is clearly defined, and the isOnboardingScreen or pageId fields help in relevant UI states.
128-133: Type definitions enhance clarity.
Combining ReduxStateProps, ReduxDispatchProps, and local props in a single type is a sound approach.
206-206: Destructuring of props is straightforward.
Good pattern for readability and maintainability.
216-216: Conditional divider rendering is neat.
Using && is a common React pattern that keeps the view concise.
Line range hint 221-247: DatasourceItem usage and event handling are well-managed.
- Event handling is properly delegated to relevant handlers, with analytics logging in place.
- Dynamic assignment of icon and name is intuitive, guiding users to create the correct data source.
- Spinner display for the
isCreatingstate is helpful feedback for users.
Also applies to: 250-251
🧰 Tools
🪛 Biome (1.9.4)
[error] 220-220: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
256-295: Functional component for scrolled focus.
The useEffect logic for focusing the component when active is true is well-structured. This improves UX by auto-scrolling for relevant sections.
372-372: Connect usage is aligned with Redux patterns.
The connect statement properly wires up state and dispatch props with the component.
| interface CreateAIPluginsProps { | ||
| pageId: string; | ||
| isCreating?: boolean; | ||
| // TODO: Fix this the next time the file is edited | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| showUnsupportedPluginDialog: (callback: any) => void; | ||
|
|
||
| plugins: Plugin[]; | ||
| // TODO: Fix this the next time the file is edited | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| createTempDatasourceFromForm: (data: any) => void; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider clarifying the any type usage.
These references reduce type safety in TypeScript. For stronger typing, define an explicit type or use generics in the function signatures.
|
Deploy-Preview-URL: https://ce-38360.dp.appsmith.com |
AmanAgarwal041
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
app/client/src/pages/Editor/IntegrationEditor/DatasourceItem.tsx (1)
10-22: Props interface is well-structured but could be improvedConsider splitting the test ID props into a separate interface for better maintainability.
interface DatasourceItem { className?: string; name: string; icon: string; description?: string; handleOnClick: () => unknown; rightSibling?: ReactNode; +} + +interface DatasourceTestIds { dataNameTestId?: string; dataCardTestId?: string; dataCardWrapperTestId?: string; dataCardDescriptionTestId?: string; dataCardImageTestId?: string; -} +} + +interface DatasourceItemProps extends DatasourceItem, Partial<DatasourceTestIds> {}app/client/src/pages/Editor/IntegrationEditor/MockDataSources.tsx (1)
101-103: Consider optimizing search performanceThe current search implementation could be optimized for large datasets.
- const filteredDatasources = mockDatasources.filter((m) => - m.name.toLocaleLowerCase().includes(searchedPlugin || ""), - ); + const searchTerm = searchedPlugin?.toLowerCase() || ""; + const filteredDatasources = searchTerm + ? mockDatasources.filter((m) => m.name.toLowerCase().includes(searchTerm)) + : mockDatasources;app/client/src/pages/Editor/IntegrationEditor/APIOrSaasPlugins.tsx (3)
47-78: Consider improving type definitionsThe interface uses
anytypes which could be replaced with more specific types for better type safety.Consider defining proper types for:
showUnsupportedPluginDialogcallbackcreateDatasourceFromFormdata parametercreateTempDatasourceFromFormdata parameter
133-170: Consider decomposing handleOnClick functionThe function handles multiple responsibilities including validation, analytics, and action dispatching.
Consider breaking it down into smaller functions:
validatePluginSupporthandleCreateDatasourcehandleCreateApi
269-322: Remove redundant double negationsThe code uses unnecessary double negations that can be simplified.
Replace:
-!!props.showSaasAPIs +Boolean(props.showSaasAPIs)app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx (2)
Line range hint
134-259: Consider extracting navigation logicThe
goToCreateDatasourcemethod contains complex navigation and validation logic.Consider extracting the navigation logic into a separate utility function:
const handleGeneratePageNavigation = ( pluginId: string, skipValidPluginCheck: boolean, generateCRUDSupportedPlugin: GenerateCRUDEnabledPluginMap, showUnsupportedPluginDialog: (callback: () => void) => void ) => { // Navigation logic here };🧰 Tools
🪛 Biome (1.9.4)
[error] 220-220: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
Line range hint
341-375: Consider improving dispatch typesThe mapDispatchToProps function uses
anytypes which could be replaced with proper action types.Consider:
- Creating proper action types for each dispatch function
- Using the Redux
Dispatchtype- Removing the eslint-disable comments after fixing the types
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/client/cypress/limited-tests.txt(1 hunks)app/client/src/pages/Editor/IntegrationEditor/APIOrSaasPlugins.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx(8 hunks)app/client/src/pages/Editor/IntegrationEditor/DatasourceItem.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/MockDataSources.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/limited-tests.txt (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
🪛 Biome (1.9.4)
app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx
[error] 220-220: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
[error] 315-315: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
[error] 322-322: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (6)
app/client/cypress/limited-tests.txt (1)
2-13: Verify test coverage for new datasource UI features
The test specifications cover essential areas including:
- Bug fixes (DS_Bug26716, DS_Bug28750)
- Schema validation
- UI features and styles
- Mobile responsiveness
✅ Verification successful
Test coverage for datasource UI features is comprehensive
The test files contain thorough coverage of:
- Datasource card UI design for mock, database, and API datasources
- Schema validation and refresh functionality
- Bug fixes for schema refresh and invalid datasource scenarios
- Schema search functionality across different datasource types
No skipped tests were found in the examined files, and all test cases appear to be actively maintained.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test coverage for datasource UI components
# Check if all new test files exist
fd -e ts -e js "DS_Bug26716_Spec|DS_Bug28750_Spec|DatasourceSchema_spec|EnableGoogle_spec|ConversionFlow_Generated_App_spec|PropertyControl_spec|EntityBottomBar_spec|JSOnLoad_cyclic_dependency_errors_spec|MockDBs_Spec|Styles_spec|Generate_Crud_New_Page_spec"
# Verify test descriptions
rg -A 5 "describe\(" "cypress/e2e/Regression/ClientSide/BugTests/DS_Bug*.ts" "cypress/e2e/Sanity/Datasources/*.ts"
Length of output: 1481
Script:
#!/bin/bash
# Let's check the test descriptions with correct paths
rg -A 5 "describe\(" "app/client/cypress/e2e/Regression/ClientSide/BugTests/DS_Bug*.ts" "app/client/cypress/e2e/Sanity/Datasources/*.ts"
# Also check test coverage for datasource UI components
rg -A 3 "it\(" "app/client/cypress/e2e/Regression/ClientSide/BugTests/DatasourceSchema_spec.ts" "app/client/cypress/e2e/Sanity/Datasources/Styles_spec.js"
# Check for any skipped tests
rg "it\.skip|describe\.skip" "app/client/cypress/e2e/Regression/ClientSide/BugTests/DS_Bug*.ts" "app/client/cypress/e2e/Sanity/Datasources/*.ts"
Length of output: 4466
app/client/src/pages/Editor/IntegrationEditor/DatasourceItem.tsx (1)
24-63: Component implementation looks good
The component is well-structured with:
- Proper default values for test IDs
- Accessibility attributes (alt text)
- Consistent class naming convention
app/client/src/pages/Editor/IntegrationEditor/MockDataSources.tsx (2)
Line range hint 27-83: MockDatasourceCard implementation looks good
The component correctly:
- Handles plugin validation
- Tracks analytics events
- Implements proper error handling
115-123: Key generation in map looks good
The key combines multiple unique identifiers to ensure uniqueness:
- datasource name
- package name
- index
app/client/src/pages/Editor/IntegrationEditor/APIOrSaasPlugins.tsx (1)
225-267: Implementation looks good!
The component follows React best practices with proper hooks usage and error handling.
app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx (1)
260-299: Implementation looks good!
The component follows React best practices with proper hooks usage and prop handling.
|
/ci-test-limit |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/12503714434. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/client/src/pages/Editor/IntegrationEditor/IntegrationStyledComponents.tsx (1)
19-26: Consider refining the grid layout implementation.The grid layout is well-structured, but there are two potential improvements:
- The border-radius property appears to have no effect without a border or background color
- The
align-items: centermight cause uneven layouts with varying content heightsexport const DatasourceContainer = styled.div` display: grid; grid-template-columns: repeat(auto-fill, minmax(218.25px, 1fr)); gap: var(--ads-v2-spaces-5); min-width: 150px; - border-radius: 4px; - align-items: center; + align-items: start; `;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/pages/Editor/IntegrationEditor/IntegrationStyledComponents.tsx(1 hunks)
🔇 Additional comments (2)
app/client/src/pages/Editor/IntegrationEditor/IntegrationStyledComponents.tsx (2)
1-18: LGTM! Good use of design tokens.
The basic styled components are well-structured and properly utilize design tokens for consistency.
50-75: LGTM! Well-structured visual components.
The visual components are well-implemented with proper use of design tokens and responsive constraints.
| export const DatasourceCard = styled.div` | ||
| display: flex; | ||
| align-items: center; | ||
| gap: 12px; | ||
| padding: 12px; | ||
| cursor: pointer; | ||
| &:hover { | ||
| background-color: var(--ads-v2-color-bg-subtle); | ||
| } | ||
|
|
||
| .cta { | ||
| display: none; | ||
| margin-right: 32px; | ||
| } | ||
|
|
||
| &:hover { | ||
| .cta { | ||
| display: flex; | ||
| } | ||
| } | ||
| `; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate hover states and use design tokens.
The DatasourceCard implementation has duplicate hover selectors and uses hardcoded values.
export const DatasourceCard = styled.div`
display: flex;
align-items: center;
- gap: 12px;
- padding: 12px;
+ gap: var(--ads-v2-spaces-3);
+ padding: var(--ads-v2-spaces-3);
cursor: pointer;
+
+ .cta {
+ display: none;
+ margin-right: var(--ads-v2-spaces-6);
+ }
+
&:hover {
background-color: var(--ads-v2-color-bg-subtle);
- }
-
- .cta {
- display: none;
- margin-right: 32px;
- }
-
- &:hover {
.cta {
display: flex;
}
}
`;📝 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 const DatasourceCard = styled.div` | |
| display: flex; | |
| align-items: center; | |
| gap: 12px; | |
| padding: 12px; | |
| cursor: pointer; | |
| &:hover { | |
| background-color: var(--ads-v2-color-bg-subtle); | |
| } | |
| .cta { | |
| display: none; | |
| margin-right: 32px; | |
| } | |
| &:hover { | |
| .cta { | |
| display: flex; | |
| } | |
| } | |
| `; | |
| export const DatasourceCard = styled.div` | |
| display: flex; | |
| align-items: center; | |
| gap: var(--ads-v2-spaces-3); | |
| padding: var(--ads-v2-spaces-3); | |
| cursor: pointer; | |
| .cta { | |
| display: none; | |
| margin-right: var(--ads-v2-spaces-6); | |
| } | |
| &:hover { | |
| background-color: var(--ads-v2-color-bg-subtle); | |
| .cta { | |
| display: flex; | |
| } | |
| } | |
| `; |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/client/cypress/locators/DatasourcesEditor.json (1)
Line range hint
1-85: Consider refactoring locators to follow Cypress best practices.Several locators in this file could be improved:
- Replace XPath selectors (e.g.,
mongoUriDropdown,basic) with data-testid attributes- Avoid text-dependent selectors (e.g.,
.t--plugin-name:contains('Users'))- Standardize the use of data-testid attributes across all locators
Example refactor for text-dependent selectors:
- "MongoDB": ".t--plugin-name:contains('MongoDB')", + "MongoDB": "[data-testid='plugin-MongoDB']",Consider organizing locators into logical groups (e.g., authentication, connection, plugins) to improve maintainability. This will make it easier to update related locators when the UI changes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/client/cypress/e2e/Sanity/Datasources/Styles_spec.js(2 hunks)app/client/cypress/locators/DatasourcesEditor.json(1 hunks)app/client/cypress/support/Pages/DataSources.ts(2 hunks)app/client/cypress/support/dataSourceCommands.js(2 hunks)app/client/src/ce/constants/messages.ts(1 hunks)app/client/src/ce/pages/Applications/CreateNewAppsOption.tsx(4 hunks)app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceHeader.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceTab.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/cypress/support/dataSourceCommands.js
- app/client/cypress/support/Pages/DataSources.ts
🧰 Additional context used
📓 Path-based instructions (2)
app/client/cypress/locators/DatasourcesEditor.json (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
app/client/cypress/e2e/Sanity/Datasources/Styles_spec.js (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
📓 Learnings (1)
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.
🔇 Additional comments (31)
app/client/cypress/locators/DatasourcesEditor.json (1)
65-65: LGTM! The selector change aligns with our guidelines.The updated selector uses the recommended data-* attribute pattern with
.t--plugin-name.app/client/cypress/e2e/Sanity/Datasources/Styles_spec.js (1)
62-62: Great job using data-testid for icon wrapper!
This approach aligns with the Cypress best practices for stable selectors.app/client/src/ce/pages/Applications/CreateNewAppsOption.tsx (4)
13-13: Nice addition of structured imports from @appsmith/ads.
Keep consistent import paths across the project for code readability.
56-58: Clear, consistent styling on the BackWrapper container.
No issues here.
67-67: Useful background color styling.
This ensures consistent theming across the app.
216-217: Good use of Flex & WithDataWrapper for layout.
The code is succinct and aligns well with the overall design approach.app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceHeader.tsx (10)
1-8: Imports look fine.
No concerns about module references or version mismatches.
9-14: Redux & styling dependencies are added correctly.
Implementation is straightforward, no issues.
15-16: Naming clarity.
CREATE_NEW_INTEGRATION_SEARCH_FORMis adequately descriptive.
17-20: Form value selector usage is appropriate.
This is a clean approach to isolate state values.
21-24: Well-defined styled component.
Flex parameters look consistent with the rest of the code.
26-33: Condensed styling for inputs.
The approach is good—makes the code more readable and maintainable.
35-37: HeaderProps interface is minimal and succinct.
No issues with property definitions.
39-40: Functional component structure is clear.
Adequately separated from Redux even before connect().
41-60: Header structure is straightforward.
Usingredux-formfor the search is a good approach.
63-72: Redundancy avoided with connect + reduxForm usage.
This pattern is a standard approach—no concerns.app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceTab.tsx (14)
21-23: APIOrSaasPlugins + DBPluginsOrMostPopular imports
These combined imports clarify plugin usage.
32-34: StyledDivider and CreateNewDatasourceHeader usage
Introduced with minimal overhead—nice.
36-45: Conditional padding for onboarding
Appropriately toggled. Code is succinct.
99-99: Boolean check for mock data
mockDataSectionVisibleis descriptive. All good.
103-106: NewIntegrationsContainer with isOnboardingScreen
Enhances layout consistency for the onboarding UI.
107-108: CreateNewDatasourceHeader addition
Centralizes new-datasource introduction.
110-115: MockDataSources gating
Perfectly checks for data presence before rendering.
116-116: DBPluginsOrMostPopular
Clear naming implies functionality coverage.
125-129: APIOrSaasPlugins
Approach for plugin separation is well-designed.
136-136: DBPluginsOrMostPopular repeated usage
Ensures DB plugin references appear again for clarity.
142-148: APIOrSaasPlugins with showSaasAPIs
Focused approach to toggle views—makes sense.
156-160: MockDataSources repeated
Great for ensuring flexible layout.
162-166: EmptySearchedPlugins fallback
Suitable placeholder for empty states.
190-191: Feature flag usage
isPremiumDatasourcesViewEnabledis well-labeled—makes feature toggles explicit.app/client/src/ce/constants/messages.ts (1)
398-410: Additional constants for new data sections.
Naming is consistent and self-explanatory.
| cy.datasourceNameStyle( | ||
| "[data-testid=database-datasource-content-wrapper] .textBtn", | ||
| ); | ||
| cy.datasourceNameStyle(".t--plugin-name"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Switch to data- attributes for improved stability*
Using a CSS class .t--plugin-name can lead to flaky tests if the class name changes. Consider using a data-* attribute for more robust test selectors.
| @@ -87,7 +81,7 @@ | |||
| //Icon wrapper | |||
| cy.datasourceIconWrapperStyle(".content-icon"); | |||
| //Name | |||
| cy.datasourceNameStyle(".t--createBlankApiCard .textBtn"); | |||
| cy.datasourceNameStyle(".t--createBlankApiCard .t--plugin-name"); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use data- attributes to avoid potential flakiness*
Selectors that rely on CSS classes (e.g., .t--createBlankApiCard .t--plugin-name) can easily break if styles change. Opt for data-* attributes to ensure consistent test behavior.
|
Deploy-Preview-URL: https://ce-38360.dp.appsmith.com |
app/client/src/pages/Editor/IntegrationEditor/APIOrSaasPlugins.tsx
Outdated
Show resolved
Hide resolved
app/client/src/pages/Editor/IntegrationEditor/APIOrSaasPlugins.tsx
Outdated
Show resolved
Hide resolved
app/client/src/pages/Editor/IntegrationEditor/APIOrSaasPlugins.tsx
Outdated
Show resolved
Hide resolved
app/client/src/pages/Editor/IntegrationEditor/AddDatasourceSecurely.tsx
Outdated
Show resolved
Hide resolved
app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceTab.tsx
Outdated
Show resolved
Hide resolved
app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx
Outdated
Show resolved
Hide resolved
| @@ -1,7 +1,7 @@ | |||
| import { getAssetUrl } from "ee/utils/airgapHelpers"; | |||
| import { ASSETS_CDN_URL } from "./ThirdPartyConstants"; | |||
|
|
|||
| interface PremiumIntegration { | |||
| export interface PremiumIntegration { | |||
There was a problem hiding this comment.
@AmanAgarwal041 Instead of creating this const file inside the common constants folder, can we create a constants folder inside IntegrationEditor and keep it there. I believe these constants are not being used outside of IntegrationEditor, that way we will have a better organisation of files, WDYT?
There was a problem hiding this comment.
Lets keep it here only, as if we keep the constants inside the IntegrationEditor then we should ideally keep the utils PremiumDatasourcesHelpers also inside IntegrationEditor. But that would not be a good option here. Keeping these things segregated is good.
There was a problem hiding this comment.
I think we should even keep utils inside IntegrationEditor, that way we would have all things needed for IntegrationEditor inside IntegrationEditor folder, following modularisation framework.
There was a problem hiding this comment.
Do we have anything adopted yet ? If not then we should leave it for now, otherwise we would be in a state where we follow two architecture mechanism at once, which becomes a little confusing.
There was a problem hiding this comment.
@AmanAgarwal041 Can you check this once? https://github.com/appsmithorg/appsmith-ee/tree/release/app/client/src/PluginActionEditor
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12595816198. |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx (1)
Line range hint
79-82: Replaceanytypes with proper TypeScript types.The TODO comments indicate technical debt in type definitions. Consider replacing these
anytypes with proper interfaces for better type safety and maintainability.Example for the callback type:
type UnsupportedPluginCallback = () => void;Also applies to: 87-90, 91-94
🧹 Nitpick comments (6)
app/client/src/pages/Editor/IntegrationEditor/AddDatasourceSecurely.tsx (1)
22-42: Consider enhancing accessibility and user preferences.While the implementation is clean, consider persisting the user's preference for hiding the security message.
Consider storing the closed state in localStorage:
- const [isClosed, setClosed] = React.useState(false); + const [isClosed, setClosed] = React.useState(() => { + return localStorage.getItem('datasource_security_message_closed') === 'true' + }); const handleClose = () => { + localStorage.setItem('datasource_security_message_closed', 'true'); setClosed(true); };app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceTab.tsx (1)
201-209: Consider using selector composition for feature flags.The feature flag checks could be composed into a single selector for better performance.
Consider creating a composed selector:
const selectDatasourceFeatureFlags = createSelector( (state: AppState) => state, (state) => ({ isRequestNewIntegrationEnabled: selectFeatureFlagCheck( state, FEATURE_FLAG.ab_request_new_integration_enabled, ), isPremiumDatasourcesViewEnabled: selectFeatureFlagCheck( state, FEATURE_FLAG.ab_premium_datasources_view_enabled, ), }) );app/client/src/pages/Editor/IntegrationEditor/APIOrSaasPlugins.tsx (1)
47-72: Enhance type safety for props interface.Consider making the callback types more specific and adding proper discriminated unions for plugin types.
type PluginParams = { skipValidPluginCheck?: boolean; pluginId?: string; type?: PluginType; }; type OnClickCallback = (actionType: keyof typeof API_ACTION, params?: PluginParams) => void;app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx (3)
220-228: Simplify conditional rendering logic.The double negation (
!!) is unnecessary and the nested ternary can be simplified.- return plugin.type === PluginType.API ? ( - !!showMostPopularPlugins ? ( +return plugin.type === PluginType.API && showMostPopularPlugins ? ( <DatasourceItem className="t--createBlankApiCard create-new-api" dataCardWrapperTestId="newapi-datasource-content-wrapper" handleOnClick={this.handleOnClick} icon={getAssetUrl(`${ASSETS_CDN_URL}/plus.png`)} key={`${plugin.id}_${idx}`} name={createMessage(CREATE_NEW_DATASOURCE_REST_API)} /> - ) : null - ) : (🧰 Tools
🪛 Biome (1.9.4)
[error] 220-220: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
266-275: Consider adding scroll behavior options.The scroll behavior is hardcoded to 'smooth'. Consider making it configurable through props for better reusability.
+ type ScrollBehavior = 'smooth' | 'auto'; + interface CreateDBOrMostPopularPluginsProps { + scrollBehavior?: ScrollBehavior; + } useEffect(() => { if (props.active && newDatasourceRef.current) { scrollIntoView(newDatasourceRef.current, { - behavior: "smooth", + behavior: props.scrollBehavior || "smooth", scrollMode: "always", block: "start", boundary: document.getElementById("new-integrations-wrapper"), }); } }, [props.active]);
315-328: Optimize plugin filtering logic.The filtering logic uses unnecessary double negations and could be simplified.
- const filteredMostPopularPlugins: Plugin[] = !!isAirgappedInstance + const filteredMostPopularPlugins: Plugin[] = isAirgappedInstance ? mostPopularPlugins.filter( (plugin: Plugin) => plugin?.packageName !== PluginPackageName.GOOGLE_SHEETS, ) : mostPopularPlugins; - let plugins = !!props?.showMostPopularPlugins + let plugins = props?.showMostPopularPlugins ? filteredMostPopularPlugins : getDBPlugins(state); plugins = plugins.filter((plugin) => - plugin.name.toLocaleLowerCase().includes(searchedPlugin || ""), + plugin.name.toLowerCase().includes((searchedPlugin || "").toLowerCase()), );🧰 Tools
🪛 Biome (1.9.4)
[error] 315-315: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
[error] 322-322: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/client/src/ce/constants/messages.ts(2 hunks)app/client/src/pages/Editor/IntegrationEditor/APIOrSaasPlugins.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/AddDatasourceSecurely.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceTab.tsx(5 hunks)app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx(8 hunks)app/client/src/pages/Editor/IntegrationEditor/IntegrationStyledComponents.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/pages/Editor/IntegrationEditor/IntegrationStyledComponents.tsx
🧰 Additional context used
📓 Learnings (1)
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.
🪛 Biome (1.9.4)
app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx
[error] 220-220: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
[error] 315-315: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
[error] 322-322: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (5)
app/client/src/pages/Editor/IntegrationEditor/AddDatasourceSecurely.tsx (1)
13-18: LGTM! Clean styling implementation.The styled component follows best practices by:
- Using design system spacing variables
- Properly extending StyledCallout
- Clean and minimal styling
app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceTab.tsx (1)
103-119: LGTM! Clean conditional rendering of mock datasources.The implementation properly handles the visibility of mock datasources based on their availability.
app/client/src/ce/constants/messages.ts (2)
398-410: LGTM! New datasource UI constants follow consistent patternsThe new constants for datasource UI elements maintain consistent naming and implementation patterns with the rest of the file.
2622-2624: LGTM! Clear security message about data handlingThe security message clearly communicates the encryption standard used (AES-256) and the data storage policy.
app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx (1)
314-314: Verify search functionality integration.Let's verify that the search functionality is properly integrated with the rest of the application.
Also applies to: 326-328
✅ Verification successful
Let me try a different approach to find the search functionality integration:
Search functionality is properly integrated
The search functionality is correctly integrated across the codebase. The
pluginSearchSelectoris:
- Defined in
CreateNewDatasourceHeader.tsx- Consistently used across multiple plugin-related components (AI, API/SaaS, DB)
- Properly imported and implemented in the reviewed file
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other components using the pluginSearchSelector rg "pluginSearchSelector" --type ts --type tsx # Check for any existing search-related tests rg -l "test.*search" --type ts --type tsxLength of output: 146
Script:
#!/bin/bash # Search for pluginSearchSelector usage with correct file type flags rg "pluginSearchSelector" -t typescript -t tsx # Check for search-related tests with correct file type flags rg -l "test.*search" -t typescript -t tsx # Look for the selector definition ast-grep --pattern 'const pluginSearchSelector = $_'Length of output: 77110
Script:
#!/bin/bash # Find files containing 'search' related functionality in selectors rg "selector.*search" -t typescript -t tsx # Find the specific file to understand the context better fd "DBOrMostPopularPlugins.tsx"Length of output: 184
Script:
#!/bin/bash # Let's try with js/ts file extensions explicitly rg "pluginSearchSelector" -g "*.js" -g "*.jsx" -g "*.ts" -g "*.tsx" # Look for search-related selectors in the specific file rg "search" app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx # Look for the form name constant that might be used with the selector rg "CREATE_NEW_INTEGRATION_SEARCH_FORM" -g "*.js" -g "*.jsx" -g "*.ts" -g "*.tsx"Length of output: 2391
| let plugins = allPlugins.filter((p) => | ||
| !props.showSaasAPIs | ||
| ? p.packageName === PluginPackageName.GRAPHQL | ||
| : p.type === PluginType.SAAS || p.type === PluginType.REMOTE, | ||
| ); | ||
|
|
||
| plugins = plugins.filter((p) => | ||
| p.name.toLocaleLowerCase().includes(searchedPlugin), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize plugin filtering performance.
The current implementation filters plugins twice. Consider combining the filters for better performance.
- let plugins = allPlugins.filter((p) =>
- !props.showSaasAPIs
- ? p.packageName === PluginPackageName.GRAPHQL
- : p.type === PluginType.SAAS || p.type === PluginType.REMOTE,
- );
-
- plugins = plugins.filter((p) =>
- p.name.toLocaleLowerCase().includes(searchedPlugin),
- );
+ const plugins = allPlugins.filter((p) => {
+ const nameMatch = p.name.toLocaleLowerCase().includes(searchedPlugin);
+ const typeMatch = !props.showSaasAPIs
+ ? p.packageName === PluginPackageName.GRAPHQL
+ : p.type === PluginType.SAAS || p.type === PluginType.REMOTE;
+ return nameMatch && typeMatch;
+ });📝 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.
| let plugins = allPlugins.filter((p) => | |
| !props.showSaasAPIs | |
| ? p.packageName === PluginPackageName.GRAPHQL | |
| : p.type === PluginType.SAAS || p.type === PluginType.REMOTE, | |
| ); | |
| plugins = plugins.filter((p) => | |
| p.name.toLocaleLowerCase().includes(searchedPlugin), | |
| ); | |
| const plugins = allPlugins.filter((p) => { | |
| const nameMatch = p.name.toLocaleLowerCase().includes(searchedPlugin); | |
| const typeMatch = !props.showSaasAPIs | |
| ? p.packageName === PluginPackageName.GRAPHQL | |
| : p.type === PluginType.SAAS || p.type === PluginType.REMOTE; | |
| return nameMatch && typeMatch; | |
| }); |
| const queryParams = getQueryParams(); | ||
| const isGeneratePageInitiator = getIsGeneratePageInitiator( | ||
| queryParams.isGeneratePageMode, | ||
| ); | ||
|
|
||
| if ( | ||
| isGeneratePageInitiator && | ||
| !params?.skipValidPluginCheck && | ||
| (!params?.pluginId || !generateCRUDSupportedPlugin[params.pluginId]) | ||
| ) { | ||
| // show modal informing user that this will break the generate flow. | ||
| props.showUnsupportedPluginDialog(() => | ||
| handleOnClick(actionType, { skipValidPluginCheck: true, ...params }), | ||
| ); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
Add error handling for invalid plugin states.
The plugin validation logic should handle edge cases more gracefully.
const handleOnClick = (
actionType: string,
params?: {
skipValidPluginCheck?: boolean;
pluginId?: string;
type?: PluginType;
},
) => {
const queryParams = getQueryParams();
const isGeneratePageInitiator = getIsGeneratePageInitiator(
queryParams.isGeneratePageMode,
);
+ if (params?.pluginId && !allPlugins.find(p => p.id === params.pluginId)) {
+ console.error(`Invalid plugin ID: ${params.pluginId}`);
+ return;
+ }
if (
isGeneratePageInitiator &&
!params?.skipValidPluginCheck &&
(!params?.pluginId || !generateCRUDSupportedPlugin[params.pluginId])
) {Committable suggestion skipped: line range outside the PR's diff.
|
/build-deploy-preview skip-tests=true |
AmanAgarwal041
added
ok-to-test
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12595971585. |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx (1)
Line range hint
69-126: Technical Debt: Replaceanytypes with proper TypeScript typesMultiple instances of
eslint-disableandanytypes should be replaced with proper TypeScript types to improve type safety and maintainability.Consider defining proper types for these callbacks and data structures:
// Instead of any, define proper types interface DBOrMostPopularPluginsProps { showUnsupportedPluginDialog: (callback: () => void) => void; } interface ReduxDispatchProps { initializeForm: (data: Record<string, unknown>) => void; createDatasource: (data: DatasourceFormData) => void; createTempDatasource: (data: { pluginId: string }) => void; }
🧹 Nitpick comments (3)
app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx (3)
216-227: Remove redundant double negationsSeveral instances of unnecessary double negation (
!!) can be simplified.- return plugin.type === PluginType.API ? ( - !!showMostPopularPlugins ? ( + return plugin.type === PluginType.API ? ( + showMostPopularPlugins ? ( - const filteredMostPopularPlugins: Plugin[] = !!isAirgappedInstance + const filteredMostPopularPlugins: Plugin[] = isAirgappedInstance - let plugins = !!props?.showMostPopularPlugins + let plugins = props?.showMostPopularPluginsAlso applies to: 312-327
🧰 Tools
🪛 Biome (1.9.4)
[error] 217-217: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
262-271: Add cleanup for scroll effectConsider adding a cleanup function to the useEffect hook to handle component unmounting.
useEffect(() => { if (props.active && newDatasourceRef.current) { - scrollIntoView(newDatasourceRef.current, { + const scrollOptions = { behavior: "smooth", scrollMode: "always", block: "start", boundary: document.getElementById("new-integrations-wrapper"), - }); + }; + const element = newDatasourceRef.current; + scrollIntoView(element, scrollOptions); + return () => { + // Cleanup any ongoing scroll animations if needed + }; } }, [props.active]);
233-237: Add error handling for analytics eventsAnalytics event logging should include error handling to prevent silent failures.
- AnalyticsUtil.logEvent("CREATE_DATA_SOURCE_CLICK", { - appName: currentApplication?.name, - pluginName: plugin.name, - pluginPackageName: plugin.packageName, - }); + try { + AnalyticsUtil.logEvent("CREATE_DATA_SOURCE_CLICK", { + appName: currentApplication?.name || "unknown", + pluginName: plugin.name, + pluginPackageName: plugin.packageName, + }); + } catch (error) { + console.error("Failed to log analytics event:", error); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx(6 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx
[error] 217-217: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
[error] 314-314: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
[error] 321-321: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
|
Deploy-Preview-URL: https://ce-38360.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
app/client/src/pages/Editor/IntegrationEditor/AIPlugins.tsx (1)
26-28: 🛠️ Refactor suggestionDefine explicit types for callback functions.
The
anytype usage reduces type safety. Consider defining proper types for these callbacks.- // TODO: Fix this the next time the file is edited - // eslint-disable-next-line @typescript-eslint/no-explicit-any - showUnsupportedPluginDialog: (callback: any) => void; + showUnsupportedPluginDialog: (callback: () => void) => void; - // TODO: Fix this the next time the file is edited - // eslint-disable-next-line @typescript-eslint/no-explicit-any - createTempDatasourceFromForm: (data: any) => void; + createTempDatasourceFromForm: (data: { pluginId: string; type: PluginType }) => void;Also applies to: 31-33
app/client/src/pages/Editor/IntegrationEditor/APIOrSaasPlugins.tsx (1)
139-150: 🛠️ Refactor suggestionAdd error handling for invalid plugin states.
The plugin validation logic should handle edge cases more gracefully.
if ( isGeneratePageInitiator && !params?.skipValidPluginCheck && (!params?.pluginId || !generateCRUDSupportedPlugin[params.pluginId]) ) { + if (params?.pluginId && !generateCRUDSupportedPlugin[params.pluginId]) { + console.error(`Plugin ${params.pluginId} does not support CRUD operations`); + } props.showUnsupportedPluginDialog(() => handleOnClick(actionType, { skipValidPluginCheck: true, ...params }), ); return; }
🧹 Nitpick comments (6)
app/client/src/pages/Editor/IntegrationEditor/MockDataSources.tsx (2)
Line range hint
32-88: Consider enhancing error handling for analytics events.The component logic is solid and the refactor to use
DatasourceItemimproves maintainability. However, consider wrapping the analytics events in a try-catch block to ensure robustness.const addMockDataSource = () => { + try { AnalyticsUtil.logEvent("ADD_MOCK_DATASOURCE_CLICK", { datasourceName: datasource.name, workspaceId, packageName: currentPlugin.packageName, pluginName: currentPlugin.name, from: DatasourceCreateEntryPoints.CREATE_NEW_DATASOURCE, }); AnalyticsUtil.logEvent("CREATE_DATA_SOURCE_CLICK", { mockDatasourceName: datasource.name, pluginName: currentPlugin.name, pluginPackageName: currentPlugin.packageName, }); + } catch (error) { + // Analytics failure shouldn't block datasource creation + console.error("Analytics event failed:", error); + } const queryParams = getQueryParams(); dispatch( addMockDatasourceToWorkspace( datasource.name, workspaceId, currentPlugin.id, currentPlugin.packageName, queryParams.isGeneratePageMode, ), ); };
96-137: Consider optimizing search performance for large lists.The search implementation is clean but could be optimized for larger datasets:
- Consider memoizing the filtered results using
useMemo- The search term transformation could be moved outside the filter loop
+ const searchTerm = React.useMemo( + () => (searchedPlugin || "").toLowerCase(), + [searchedPlugin] + ); + const filteredDatasources = React.useMemo( + () => + mockDatasources.filter((m) => + m.name.toLowerCase().includes(searchTerm) + ), + [mockDatasources, searchTerm] + );app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceHeader.tsx (1)
21-32: Consider using design tokens for dimensions.Replace the magic number
28pxwith a design token for better maintainability and consistency.- height: 28px; + height: var(--ads-v2-spaces-7);app/client/src/pages/Editor/IntegrationEditor/AIPlugins.tsx (1)
71-71: Consider early return pattern for better readability.The condition can be simplified using early return pattern.
- if (isAirgappedInstance || props.plugins.length === 0) return null; + if (isAirgappedInstance) return null; + if (props.plugins.length === 0) return null;app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx (2)
217-217: Remove redundant double negations.The static analysis tool correctly identified unnecessary double negations.
- !!showMostPopularPlugins + Boolean(showMostPopularPlugins) - !!isAirgappedInstance + Boolean(isAirgappedInstance) - !!props?.showMostPopularPlugins + Boolean(props?.showMostPopularPlugins)Also applies to: 316-316, 323-323
🧰 Tools
🪛 Biome (1.9.4)
[error] 217-217: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
Line range hint
134-251: Consider converting class component to functional component.The
DBOrMostPopularPluginsclass component could be converted to a functional component for consistency with modern React patterns.Would you like me to help generate a functional component version of this class?
🧰 Tools
🪛 Biome (1.9.4)
[error] 217-217: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
[error] 316-316: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
[error] 323-323: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/client/src/ce/constants/messages.ts(2 hunks)app/client/src/constants/PremiumDatasourcesConstants.ts(2 hunks)app/client/src/pages/Editor/IntegrationEditor/AIPlugins.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/APIOrSaasPlugins.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceHeader.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx(6 hunks)app/client/src/pages/Editor/IntegrationEditor/EmptySearchedPlugins.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/MockDataSources.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/client/src/pages/Editor/IntegrationEditor/EmptySearchedPlugins.tsx
- app/client/src/constants/PremiumDatasourcesConstants.ts
- app/client/src/ce/constants/messages.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx
[error] 217-217: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
[error] 316-316: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
[error] 323-323: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (7)
app/client/src/pages/Editor/IntegrationEditor/MockDataSources.tsx (1)
12-25: LGTM! Clean import organization and well-structured interface.The imports are properly organized and the new
MockDataSourcesPropsinterface is well-defined with appropriate optional flags.Also applies to: 90-94
app/client/src/pages/Editor/IntegrationEditor/CreateNewDatasourceHeader.tsx (4)
17-19: LGTM!The Redux Form selector implementation follows best practices.
62-71: LGTM!The Redux connection and form configuration follow best practices.
34-36: Verify props usage.The
HeaderPropsinterface defines asearchprop, but the component doesn't use it. Either remove the unused prop or utilize it in the component.
1-15: Verify enterprise edition imports.The component is importing from enterprise edition paths (
ee/reducersandee/constants/messages). Ensure this is intentional and consider if a community edition equivalent is needed.app/client/src/pages/Editor/IntegrationEditor/AIPlugins.tsx (1)
4-4: Add unit tests as indicated by TODO comment.The TODO comment indicates missing tests for this functionality.
Would you like me to help generate unit tests for this component?
app/client/src/pages/Editor/IntegrationEditor/APIOrSaasPlugins.tsx (1)
282-290: Optimize plugin filtering performance.The current implementation filters plugins twice. Consider combining the filters for better performance.
- let plugins = allPlugins.filter((p) => - !props.showSaasAPIs - ? p.packageName === PluginPackageName.GRAPHQL - : p.type === PluginType.SAAS || p.type === PluginType.REMOTE, - ); - - plugins = plugins.filter((p) => - p.name.toLocaleLowerCase().includes(searchedPlugin), - ); + const plugins = allPlugins.filter((p) => { + const typeMatch = !props.showSaasAPIs + ? p.packageName === PluginPackageName.GRAPHQL + : p.type === PluginType.SAAS || p.type === PluginType.REMOTE; + const nameMatch = p.name.toLocaleLowerCase().includes(searchedPlugin); + return typeMatch && nameMatch; + });
| const CreateNewDatasourceHeader = () => { | ||
| return ( | ||
| <Flex alignItems="flex-end" gap="spaces-5"> | ||
| <HeaderText flexDirection="column"> | ||
| <Text kind="heading-l"> | ||
| {createMessage(CONNECT_A_DATASOURCE_HEADING)} | ||
| </Text> | ||
| <Text>{createMessage(CONNECT_A_DATASOURCE_SUBHEADING)}</Text> | ||
| </HeaderText> | ||
| <SearchContainer justifyContent="flex-end"> | ||
| <Field | ||
| component={ReduxFormTextField} | ||
| name="search" | ||
| placeholder={createMessage(SEARCH_FOR_DATASOURCES)} | ||
| size="md" | ||
| startIcon="search-line" | ||
| style={{ maxWidth: "480px" }} | ||
| type="search" | ||
| /> | ||
| </SearchContainer> | ||
| </Flex> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add accessibility attributes to search field.
The search field should have appropriate ARIA attributes for better accessibility.
<Field
component={ReduxFormTextField}
name="search"
+ aria-label={createMessage(SEARCH_FOR_DATASOURCES)}
placeholder={createMessage(SEARCH_FOR_DATASOURCES)}
size="md"
startIcon="search-line"📝 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.
| const CreateNewDatasourceHeader = () => { | |
| return ( | |
| <Flex alignItems="flex-end" gap="spaces-5"> | |
| <HeaderText flexDirection="column"> | |
| <Text kind="heading-l"> | |
| {createMessage(CONNECT_A_DATASOURCE_HEADING)} | |
| </Text> | |
| <Text>{createMessage(CONNECT_A_DATASOURCE_SUBHEADING)}</Text> | |
| </HeaderText> | |
| <SearchContainer justifyContent="flex-end"> | |
| <Field | |
| component={ReduxFormTextField} | |
| name="search" | |
| placeholder={createMessage(SEARCH_FOR_DATASOURCES)} | |
| size="md" | |
| startIcon="search-line" | |
| style={{ maxWidth: "480px" }} | |
| type="search" | |
| /> | |
| </SearchContainer> | |
| </Flex> | |
| ); | |
| }; | |
| const CreateNewDatasourceHeader = () => { | |
| return ( | |
| <Flex alignItems="flex-end" gap="spaces-5"> | |
| <HeaderText flexDirection="column"> | |
| <Text kind="heading-l"> | |
| {createMessage(CONNECT_A_DATASOURCE_HEADING)} | |
| </Text> | |
| <Text>{createMessage(CONNECT_A_DATASOURCE_SUBHEADING)}</Text> | |
| </HeaderText> | |
| <SearchContainer justifyContent="flex-end"> | |
| <Field | |
| component={ReduxFormTextField} | |
| name="search" | |
| aria-label={createMessage(SEARCH_FOR_DATASOURCES)} | |
| placeholder={createMessage(SEARCH_FOR_DATASOURCES)} | |
| size="md" | |
| startIcon="search-line" | |
| style={{ maxWidth: "480px" }} | |
| type="search" | |
| /> | |
| </SearchContainer> | |
| </Flex> | |
| ); | |
| }; |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| showUnsupportedPluginDialog: (callback: any) => void; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace any types with proper TypeScript types.
Several TODO comments indicate the need to fix any types. Consider defining proper types for better type safety.
- // TODO: Fix this the next time the file is edited
- // eslint-disable-next-line @typescript-eslint/no-explicit-any
- showUnsupportedPluginDialog: (callback: any) => void;
+ showUnsupportedPluginDialog: (callback: () => void) => void;
- const mapDispatchToProps = (dispatch: any) => {
+ const mapDispatchToProps = (dispatch: ThunkDispatch<AppState, null, AnyAction>) => {Also applies to: 344-353
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12597718048. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
app/client/src/pages/Editor/IntegrationEditor/AIPlugins.tsx (3)
32-62: Consider memoizing the onClick handlerThe
handleOnClickfunction is recreated on every render. Consider usinguseCallbackfor better performance, especially if this component re-renders frequently.- const handleOnClick = (plugin: Plugin) => { + const handleOnClick = useCallback((plugin: Plugin) => { AnalyticsUtil.logEvent("CREATE_DATA_SOURCE_CLICK", { pluginName: plugin.name, pluginPackageName: plugin.packageName, }); props.createTempDatasourceFromForm({ pluginId: plugin.id, type: plugin.type, }); - }; + }, [props.createTempDatasourceFromForm]);
64-80: Consider extracting airgapped check to a custom hookThe airgapped check could be reused across components. Consider extracting it to a custom hook for better reusability and separation of concerns.
+const useAirgappedStatus = () => { + return { + isAirgappedInstance: isAirgapped(), + }; +}; function CreateAIPlugins(props: CreateAIPluginsProps) { - const isAirgappedInstance = isAirgapped(); + const { isAirgappedInstance } = useAirgappedStatus();
82-110: Consider memoizing filtered pluginsThe plugins filtering and sorting could benefit from memoization to prevent unnecessary recalculations.
+import { createSelector } from 'reselect'; +const selectFilteredPlugins = createSelector( + [getPlugins, pluginSearchSelector], + (plugins, search) => { + const searchedPlugin = (search || "").toLowerCase(); + return plugins + .sort((a, b) => a.name.localeCompare(b.name)) + .filter( + (plugin) => + plugin.type === PluginType.AI && + plugin.name.toLowerCase().includes(searchedPlugin) + ); + } +); const mapStateToProps = (state: AppState) => { - const searchedPlugin = (pluginSearchSelector(state, "search") || "").toLowerCase(); - let plugins = getPlugins(state); - plugins = plugins - .sort((a, b) => a.name.localeCompare(b.name)) - .filter( - (plugin) => - plugin.type === PluginType.AI && - plugin.name.toLowerCase().includes(searchedPlugin) - ); return { - plugins, + plugins: selectFilteredPlugins(state), }; };app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx (5)
Line range hint
52-71: Optimize removeQueryParams functionThe current implementation can be simplified using URLSearchParams for better maintainability and efficiency.
const removeQueryParams = (paramKeysToRemove: Array<string>) => { - const queryParams = getQueryParams(); - let queryString = ""; - const queryParamKeys = Object.keys(queryParams); - - if (queryParamKeys && queryParamKeys.length) { - queryParamKeys.map((key) => { - if (!paramKeysToRemove.includes(key)) { - queryString += - encodeURIComponent(key) + "=" + encodeURIComponent(queryParams[key]); - } - }); - - return "?" + queryString; - } - - return ""; + const params = new URLSearchParams(getQueryParams()); + paramKeysToRemove.forEach(key => params.delete(key)); + const queryString = params.toString(); + return queryString ? `?${queryString}` : ""; };
212-214: Remove redundant double negationsThe double negation (
!!) is unnecessary and can be simplified.- return plugin.type === PluginType.API ? ( - !!showMostPopularPlugins ? ( + return plugin.type === PluginType.API ? ( + showMostPopularPlugins ? (🧰 Tools
🪛 Biome (1.9.4)
[error] 213-213: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
Line range hint
127-252: Consider adding error boundariesThe component handles complex datasource creation logic. Consider wrapping it with an error boundary to gracefully handle potential runtime errors.
class DatasourceErrorBoundary extends React.Component { componentDidCatch(error: Error) { // Handle error logging AnalyticsUtil.logEvent("DATASOURCE_CREATION_ERROR", { error: error.message, }); } render() { return this.props.children; } }🧰 Tools
🪛 Biome (1.9.4)
[error] 213-213: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
[error] 313-313: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
[error] 320-320: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
253-295: Extract scroll logic to a custom hookConsider extracting the scroll logic into a reusable custom hook for better maintainability.
const useScrollIntoView = (ref: RefObject<HTMLElement>, active?: boolean) => { useEffect(() => { if (active && ref.current) { scrollIntoView(ref.current, { behavior: "smooth", scrollMode: "always", block: "start", boundary: document.getElementById("new-integrations-wrapper"), }); } }, [active, ref]); };
310-326: Optimize plugin filtering and remove double negationsThe plugin filtering logic can be optimized and simplified.
- const searchedPlugin = ( - pluginSearchSelector(state, "search") || "" - ).toLocaleLowerCase(); - const filteredMostPopularPlugins: Plugin[] = !!isAirgappedInstance + const searchedPlugin = (pluginSearchSelector(state, "search") || "").toLowerCase(); + const filteredMostPopularPlugins: Plugin[] = isAirgappedInstance ? mostPopularPlugins.filter( (plugin: Plugin) => plugin?.packageName !== PluginPackageName.GOOGLE_SHEETS, ) : mostPopularPlugins; - let plugins = !!props?.showMostPopularPlugins + let plugins = props?.showMostPopularPlugins ? filteredMostPopularPlugins : getDBPlugins(state); plugins = plugins.filter((plugin) => - plugin.name.toLocaleLowerCase().includes(searchedPlugin), + plugin.name.toLowerCase().includes(searchedPlugin), );🧰 Tools
🪛 Biome (1.9.4)
[error] 313-313: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
[error] 320-320: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/pages/Editor/IntegrationEditor/AIPlugins.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx(8 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx
[error] 213-213: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
[error] 313-313: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
[error] 320-320: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (2)
app/client/src/pages/Editor/IntegrationEditor/AIPlugins.tsx (1)
23-30: Props interface looks good!Clean and well-typed interface definition with all necessary props.
app/client/src/pages/Editor/IntegrationEditor/DBOrMostPopularPlugins.tsx (1)
Line range hint
72-125: Interface definitions look good!Clean and well-structured TypeScript interfaces with proper type definitions.
|
Deploy-Preview-URL: https://ce-38360.dp.appsmith.com |
Description
The PR
Searchbar,Titleand updates design ofBanneron datasource home pageFixes #38209
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.All"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12597711159
Commit: 3ec726c
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ClientSide/Linting/BasicLint_spec.ts
List of identified flaky tests.Fri, 03 Jan 2025 13:47:22 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
DatasourceItemcomponent for displaying individual datasource information.EmptySearchedPluginscomponent to handle empty search states effectively.Bug Fixes
Refactor
Style
Chores