chore: Replacing entity item component in schema tables with the new List item component in ADS#38987
Conversation
WalkthroughThis pull request introduces several targeted changes across the UI components. The styling of Datasource tabs has been updated with modified CSS selectors and spacing adjustments. The plugin filtering logic in GlobalSearchHooks has been refined by leveraging a new type. Additionally, the Datasource structure rendering has been revamped by replacing the Entity component with a ListItem from the ADS library and removing virtualization, and a minor change was made to the datasource icon export. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as GlobalSearch UI
participant Handler as updateActionOperations
participant PluginList as Plugin Collection
UI->>Handler: Initiate update of action operations
Handler->>PluginList: Filter plugins (type: API & uiComponent: ApiEditorForm)
PluginList-->>Handler: Return matching plugin (if exists)
Handler->>UI: Update action operation with pluginId
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
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
IDE Pod
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13120574557. |
|
Deploy-Preview-URL: https://ce-38987.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13121666939. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
app/client/src/pages/Editor/DatasourceInfo/DatasourceStructure.tsx (1)
92-94: Address TODO and type safety concerns.The TODO comment with eslint-disable for explicit any type should be addressed. Consider typing the event parameter properly.
- // TODO: Fix this the next time the file is edited - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const onEntityClick = (entity: any) => { + const onEntityClick = (entity: React.MouseEvent<HTMLDivElement>) => {
🧹 Nitpick comments (1)
app/client/src/pages/Editor/DatasourceInfo/DatasourceStructure.tsx (1)
175-184: Consider performance implications of removing virtualization.Replacing virtualization with direct mapping could impact performance when rendering large datasets. Consider keeping virtualization for better performance or document the maximum expected number of items.
If the number of tables can be large, consider using a virtualized list library like
react-windoworreact-virtualized.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/pages/Editor/DatasourceInfo/DatasourceStructure.tsx(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-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-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (3)
app/client/src/pages/Editor/DatasourceInfo/DatasourceStructure.tsx (3)
13-21: LGTM! Import changes align with PR objectives.The imports from @appsmith/ads are appropriate for the new List item component implementation.
150-158: LGTM! ListItem implementation is clean.The ListItem props are well-mapped and maintain the required functionality from the previous Entity component.
57-59: Verify the visual impact of padding removal.The removal of padding might affect the visual spacing of list items. Please ensure this doesn't create any alignment or spacing issues with the new List component.
|
Deploy-Preview-URL: https://ce-38987.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13122482196. |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
app/client/src/pages/Editor/DatasourceInfo/DatasourceStructure.tsx (1)
92-94: Fix TODO and improve type safety.The TODO comment and any type should be addressed.
- // TODO: Fix this the next time the file is edited - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const onEntityClick = (entity: any) => { + const onEntityClick = (event: React.MouseEvent<HTMLDivElement>) => {
🧹 Nitpick comments (1)
app/client/src/pages/Editor/DatasourceInfo/DatasourceStructure.tsx (1)
1-2: Consider removing unnecessary EntityClassNames import.The EntityClassNames import is only used for a CSS class name string. Consider defining this locally since we're moving away from the Entity component.
-import { EntityClassNames } from "../Explorer/Entity"; +const CONTEXT_MENU_CLASS = "t--template-menu-trigger";
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/packages/design-system/ads/src/List/List.tsx(1 hunks)app/client/packages/design-system/ads/src/List/List.types.tsx(1 hunks)app/client/src/pages/Editor/DatasourceInfo/DatasourceStructure.tsx(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- 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-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (5)
app/client/packages/design-system/ads/src/List/List.types.tsx (1)
39-40: LGTM! Well-documented test ID property.The addition of the optional
dataTestIdproperty follows the existing pattern and is properly documented.app/client/packages/design-system/ads/src/List/List.tsx (1)
127-127: LGTM! Proper implementation of test ID attribute.The data-testid attribute is correctly implemented using the new prop.
app/client/src/pages/Editor/DatasourceInfo/DatasourceStructure.tsx (3)
57-59: LGTM!The StyledList component is appropriately styled for the list container.
150-158: LGTM! Clean transition to ListItem component.The ListItem implementation maintains feature parity with the previous Entity component while providing a cleaner interface.
175-184: Consider performance impact of removing virtualization.Removing the Virtuoso component means all items will be rendered at once. This could impact performance for large datasets.
Let's check if there are any datasources with large numbers of tables:
✅ Verification successful
Performance Impact Consideration: Removal of Virtuoso vs. Real-World Data Sizes
Based on our search, the "tables" arrays in the codebase appear mainly in unit test data (e.g., the references in
unitTestUtils.tsandHideGeneratePageButton.test.tsx) and do not reveal evidence of production datasources with a large number of tables. While we acknowledge that rendering all items at once could impact performance if a datasource were to include many tables, current usage seems limited to small datasets. Still, keep an eye on performance if you anticipate datasources growing significantly.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for table array definitions or assignments rg -A 5 "tables:\s*\[|tables\s*=\s*\[" --type typescriptLength of output: 93
Script:
#!/bin/bash # Re-run search for table array definitions or assignments in TS/TSX files using glob patterns instead of file type rg -A 5 "tables:\s*\[|tables\s*=\s*\[" --glob "*.ts" --glob "*.tsx"Length of output: 2775
|
Deploy-Preview-URL: https://ce-38987.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13130579582. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/pages/Editor/DatasourceInfo/DatasourceStructure.tsx (2)
2-2: Consider removing unused EntityClassNames import.The
EntityClassNamesimport is only used for a CSS class name. Consider moving this class name to a constants file or updating it to match the new ListItem component naming convention.
146-154: Update className to match new component.The className still references the old entity component (
t--entity-item). Consider updating it to match the new ListItem component convention for consistency.- className={`datasourceStructure-${props.context} t--entity-item`} + className={`datasourceStructure-${props.context} t--list-item`}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/pages/Editor/DatasourceInfo/DatasourceStructure.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: storybook-tests
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
🔇 Additional comments (1)
app/client/src/pages/Editor/DatasourceInfo/DatasourceStructure.tsx (1)
184-188: LGTM! Virtualization correctly maintained.Good job maintaining the Virtuoso implementation for performance optimization, as highlighted in previous reviews.
|
Deploy-Preview-URL: https://ce-38987.dp.appsmith.com |
…List item component in ADS (appsmithorg#38987) ## Description Replacing entity item component in schema tables with the new List item component in ADS Fixes [appsmithorg#38292](appsmithorg#38292) ## Automation /ok-to-test tags="@tag.All" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/13130548638> > Commit: a280e47 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13130548638&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Tue, 04 Feb 2025 09:17:45 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Improved API action selection with refined filtering to ensure accurate plugin association. - **Refactor** - Updated the datasource structure view for a streamlined and simplified list presentation. - **Style** - Modernized styling across the datasource tab and schema view, including adjusted spacing. - Enhanced icon appearance for more consistent visual cues. - Added testability attributes to list items for improved automated testing support. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
Replacing entity item component in schema tables with the new List item component in ADS
Fixes #38292
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13130548638
Commit: a280e47
Cypress dashboard.
Tags:
@tag.AllSpec:
Tue, 04 Feb 2025 09:17:45 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Refactor
Style