chore: Updating generate page interaction to show it in a modal following the IDE 2.0 interaction pattern #37414
Conversation
WalkthroughThe changes in this pull request primarily involve the removal of deprecated constants and functions related to generating template forms, transitioning to a modal-based approach for page generation. New action types and modal management functionality have been introduced, enhancing the Redux state management for the generation process. Additionally, several components have been updated to reflect these changes, including the integration of new selectors and modifications to existing rendering logic, while ensuring that the overall structure and functionality of the application remain intact. 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
|
|
This PR has increased the number of cyclic dependencies by 2, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
![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
🧹 Outside diff range and nitpick comments (16)
app/client/src/pages/Editor/GeneratePage/helpers.ts (1)
1-2: Consider moving enterprise-specific imports to a separate fileThe ReduxActionTypes import is from the enterprise edition folder. For better maintainability and separation of concerns, consider moving enterprise-specific action types to a common constants file.
app/client/src/pages/Templates/TemplatesModal/Header.tsx (1)
Line range hint
1-24: Consider adding prop documentation.While the component is simple, adding JSDoc comments for the
TemplateModalHeaderPropsinterface would improve maintainability.+/** + * Props for the TemplateModalHeader component + * @param className - Optional CSS class name for styling + */ interface TemplateModalHeaderProps { className?: string; }app/client/src/selectors/pageListSelectors.tsx (1)
19-24: Consider using createSelector for performance optimizationThe new selectors access state directly without memoization. Consider using
createSelectorto prevent unnecessary re-renders, maintaining consistency with other selectors in this file.Here's how you could refactor these selectors:
-export const getIsGeneratePageModalOpen = (state: AppState) => - state.entities.pageList.isGeneratePageModalOpen?.value; +export const getIsGeneratePageModalOpen = createSelector( + getPageListState, + (pageList: PageListReduxState) => pageList.isGeneratePageModalOpen?.value, +); -export const getGeneratePageModalParams = (state: AppState) => - state.entities.pageList.isGeneratePageModalOpen?.params; +export const getGeneratePageModalParams = createSelector( + getPageListState, + (pageList: PageListReduxState) => pageList.isGeneratePageModalOpen?.params, +);app/client/src/sagas/SaaSPaneSagas.ts (1)
81-96: Consider enhancing error handling and documentation.
- The comments could be structured as JSDoc for better IDE integration
- Consider adding error handling for the modal opening action
- // isGeneratePageInitiator ensures that datasource is being created from generate page with data - // then we check if the current plugin is supported for generate page with data functionality - // and finally isDBCreated ensures that datasource is not in temporary state and - // user has explicitly saved the datasource, before redirecting back to generate page + /** + * Opens the generate page modal when: + * 1. Datasource is created from generate page with data + * 2. Plugin supports generate page functionality + * 3. Datasource is saved and not in temporary state + */ if ( isGeneratePageInitiator && updatedDatasource.pluginId && generateCRUDSupportedPlugin[updatedDatasource.pluginId] && isDBCreated ) { - yield put( - openGeneratePageModal({ - datasourceId: updatedDatasource.id, - }), - ); + try { + yield put( + openGeneratePageModal({ + datasourceId: updatedDatasource.id, + }), + ); + } catch (error) { + // Handle modal opening failure + console.error('Failed to open generate page modal:', error); + } }app/client/src/ce/hooks/datasourceEditorHooks.tsx (2)
Line range hint
49-102: Consider improving type safety and reducing complexity.
- The event handler uses
anytype which could be replaced with a proper React.MouseEvent type.- Complex boolean conditions could be extracted into named variables for better readability.
- onClick={(e: any) => { + onClick={(e: React.MouseEvent<HTMLButtonElement>) => {
Line range hint
127-141: Consider extracting button component for better maintainability.The generate page button logic contains complex conditional rendering and event handling. Consider extracting it into a separate component to improve code organization and reusability.
Example structure:
interface GeneratePageButtonProps { isDisabled: boolean; onClick: (e: React.MouseEvent<HTMLButtonElement>) => void; } const GeneratePageButton: React.FC<GeneratePageButtonProps> = ({ isDisabled, onClick }) => ( <Button className="t--generate-template" isDisabled={isDisabled} kind="secondary" onClick={onClick} size="md" > {createMessage(GENERATE_NEW_PAGE_BUTTON_TEXT)} </Button> );app/client/src/reducers/entityReducers/pageListReducer.tsx (3)
23-26: Consider making the modal state non-optionalSince the initial state provides default values, consider making
isGeneratePageModalOpennon-optional in the interface to avoid unnecessary null checks.
242-262: Enhance modal action handlersTwo suggestions for improvement:
- The
|| {}in OPEN handler is redundant since action.payload is already typed as GeneratePageModalParams- Consider clearing params on modal close to prevent stale data
[ReduxActionTypes.SET_GENERATE_PAGE_MODAL_OPEN]: ( state: PageListReduxState, action: ReduxAction<GeneratePageModalParams>, ) => ({ ...state, isGeneratePageModalOpen: { value: true, - params: action.payload || {}, + params: action.payload, }, }), [ReduxActionTypes.SET_GENERATE_PAGE_MODAL_CLOSE]: ( state: PageListReduxState, ) => ({ ...state, isGeneratePageModalOpen: { - ...state.isGeneratePageModalOpen, value: false, + params: {}, }, }),
327-330: Follow TypeScript naming conventionsThe property
new_pageuses snake_case. Consider using camelCase to maintain consistency with TypeScript conventions.export interface GeneratePageModalParams { datasourceId?: string; - new_page?: boolean; + newPage?: boolean; }app/client/src/sagas/QueryPaneSagas.ts (1)
488-499: Consider refactoring the conditional logic for better readabilityThe complex condition could be extracted into a helper function to improve readability and maintainability.
Consider this refactor:
+ const shouldOpenGeneratePageModal = ( + isGeneratePageInitiator: boolean, + datasource: any, + generateCRUDSupportedPlugin: Record<string, any>, + isDBCreated: boolean, + ) => { + return ( + isGeneratePageInitiator && + datasource.pluginId && + generateCRUDSupportedPlugin[datasource.pluginId] && + isDBCreated + ); + }; - if ( - isGeneratePageInitiator && - updatedDatasource.pluginId && - generateCRUDSupportedPlugin[updatedDatasource.pluginId] && - isDBCreated - ) { + if (shouldOpenGeneratePageModal( + isGeneratePageInitiator, + updatedDatasource, + generateCRUDSupportedPlugin, + isDBCreated + )) { yield put( openGeneratePageModal({ datasourceId: updatedDatasource.id, }), ); }app/client/src/pages/Editor/GeneratePage/index.tsx (2)
11-13: Combine imports from "react-redux" into a single statement.Apply this diff to improve code clarity:
-import { useSelector } from "react-redux"; -import { useDispatch } from "react-redux"; +import { useSelector, useDispatch } from "react-redux";
36-38: Localize the hard-coded string for internationalization support.Consider using
createMessageto localize the text:- <Text renderAs="p"> - Auto create a simple CRUD interface on top of your data - </Text> + <Text renderAs="p"> + {createMessage(GENERATE_PAGE_FORM_DESCRIPTION)} + </Text>Add the following to your messages file:
export const GENERATE_PAGE_FORM_DESCRIPTION = "Auto create a simple CRUD interface on top of your data";app/client/src/pages/Editor/GeneratePage/components/GeneratePageForm/GeneratePageForm.tsx (3)
702-703: Resolve the TODO regarding the 'virtual' prop in the Select componentThe 'virtual' prop is set to
falsewith a TODO comment. Re-enabling virtualization can improve performance when dealing with large datasets.Let me know if you need assistance fixing the virtualization issue.
563-563: Extract 'generate-page' string into a constantThe string
'generate-page'is used in multiple places as a parameter. Extracting it into a constant will enhance maintainability and reduce the risk of typos.Apply this diff to define a constant and use it:
+const GENERATE_PAGE_MODE = "generate-page"; ... // In routeToCreateNewDatasource function history.push( integrationEditorURL({ basePageId, selectedTab: INTEGRATION_TABS.NEW, params: { isGeneratePageMode: GENERATE_PAGE_MODE }, }), ); ... // In goToEditDatasource function const redirectURL = datasourcesEditorIdURL({ basePageId, datasourceId: selectedDatasource.id as string, params: { isGeneratePageMode: GENERATE_PAGE_MODE }, });Also applies to: 606-606
Line range hint
532-546: Add missing dependency to theuseEffecthookThe
currentModevariable is being updated inside theuseEffecthook but is not included in the dependency array. AlthoughuseRefvalues don't cause re-renders, it's good practice to include all used variables.Consider adding
currentModeto the dependency array for clarity.app/client/src/sagas/DatasourcesSagas.ts (1)
342-373: Consider refactoring 'openGeneratePageModal' logic into a shared helper functionThe conditional logic for dispatching
openGeneratePageModalbased onisGeneratePageInitiatoris duplicated in bothaddMockDbToDatasourcesandupdateDatasourceSuccessSaga. Extracting this logic into a shared helper function can reduce code duplication and enhance maintainability.Also applies to: 1540-1551
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (20)
app/client/src/ce/RouteBuilder.ts(0 hunks)app/client/src/ce/constants/ReduxActionConstants.tsx(3 hunks)app/client/src/ce/constants/messages.ts(1 hunks)app/client/src/ce/constants/routes/appRoutes.ts(0 hunks)app/client/src/ce/hooks/datasourceEditorHooks.tsx(3 hunks)app/client/src/ce/pages/Editor/IDE/MainPane/useRoutes.tsx(0 hunks)app/client/src/pages/Editor/DataSourceEditor/DatasourceSection.tsx(0 hunks)app/client/src/pages/Editor/Explorer/Pages/AddPageContextMenu.tsx(2 hunks)app/client/src/pages/Editor/GeneratePage/components/GeneratePageForm/GeneratePageForm.tsx(9 hunks)app/client/src/pages/Editor/GeneratePage/components/PageContent.tsx(0 hunks)app/client/src/pages/Editor/GeneratePage/helpers.ts(1 hunks)app/client/src/pages/Editor/GeneratePage/index.tsx(1 hunks)app/client/src/pages/Editor/IntegrationEditor/DatasourceCard.tsx(2 hunks)app/client/src/pages/Editor/index.tsx(2 hunks)app/client/src/pages/Templates/TemplatesModal/Header.tsx(1 hunks)app/client/src/reducers/entityReducers/pageListReducer.tsx(4 hunks)app/client/src/sagas/DatasourcesSagas.ts(3 hunks)app/client/src/sagas/QueryPaneSagas.ts(2 hunks)app/client/src/sagas/SaaSPaneSagas.ts(3 hunks)app/client/src/selectors/pageListSelectors.tsx(1 hunks)
💤 Files with no reviewable changes (5)
- app/client/src/ce/RouteBuilder.ts
- app/client/src/ce/constants/routes/appRoutes.ts
- app/client/src/ce/pages/Editor/IDE/MainPane/useRoutes.tsx
- app/client/src/pages/Editor/DataSourceEditor/DatasourceSection.tsx
- app/client/src/pages/Editor/GeneratePage/components/PageContent.tsx
✅ Files skipped from review due to trivial changes (1)
- app/client/src/ce/constants/messages.ts
🔇 Additional comments (20)
app/client/src/pages/Editor/GeneratePage/helpers.ts (3)
4-9: LGTM! Clean Redux action creator implementation
The function follows Redux best practices with proper typing and optional payload handling.
1-15: Verify complete migration from URL-based to modal-based approach
Since this is part of a transition from URL-based to modal-based page generation, let's ensure all previous URL-based implementations have been properly migrated.
#!/bin/bash
# Description: Check for any remaining URL-based page generation implementations
# Search for the old generateTemplateFormURL usage
rg "generateTemplateFormURL" --type ts
# Search for potential URL patterns that might still be using the old approach
rg -i "generate.*page.*url" --type ts11-15: Verify reducer handling for modal close action
The action creator implementation looks good, but let's ensure the reducer properly handles this action type.
✅ Verification successful
Action creator and reducer implementation verified
The reducer in pageListReducer.tsx properly handles the SET_GENERATE_PAGE_MODAL_CLOSE action by setting isGeneratePageModalOpen.value to false, maintaining the expected Redux state management pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the reducer properly handles the modal close action
# Look for the reducer that handles these modal actions
# Search for the reducer file handling these action types
rg -l "SET_GENERATE_PAGE_MODAL_CLOSE" --type ts
# Check the reducer implementation
ast-grep --pattern 'case $action: {
$$$
return {
$$$
};
}'
Length of output: 331
Script:
#!/bin/bash
# Search for reducer handling the generate page modal state
rg -l "SET_GENERATE_PAGE_MODAL" --type ts
# If found, examine the reducer implementation
rg -A 10 "SET_GENERATE_PAGE_MODAL_CLOSE" --type ts
# Also check for any constants/types definition
rg "SET_GENERATE_PAGE_MODAL" --type ts
Length of output: 3125
app/client/src/pages/Templates/TemplatesModal/Header.tsx (1)
20-20: LGTM! Clean and simplified implementation.
The direct usage of createMessage simplifies the component and removes unnecessary complexity.
Let's verify the message constant is properly defined:
✅ Verification successful
Constant is properly defined and used correctly
The ADD_PAGE_FROM_TEMPLATE_MODAL.title constant is defined in app/client/src/ce/constants/messages.ts and returns the expected modal title string.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the ADD_PAGE_FROM_TEMPLATE_MODAL.title constant exists
# Expected: Should find the constant definition
rg -A 2 "ADD_PAGE_FROM_TEMPLATE_MODAL.*=.*{" "ee/constants/messages"
Length of output: 200
Script:
#!/bin/bash
# Search for ADD_PAGE_FROM_TEMPLATE_MODAL constant definition
rg "ADD_PAGE_FROM_TEMPLATE_MODAL" --type ts
# Also check in constants directory specifically
fd "messages" -t f -e ts -e js | xargs rg "ADD_PAGE_FROM_TEMPLATE_MODAL"
Length of output: 714
Script:
#!/bin/bash
# Get the content of the messages constant to verify the title property
rg -A 5 "ADD_PAGE_FROM_TEMPLATE_MODAL = {" app/client/src/ce/constants/messages.ts
Length of output: 212
Script:
#!/bin/bash
# Get the content of the messages file to verify the constant
cat app/client/src/ce/constants/messages.ts | grep -A 5 "ADD_PAGE_FROM_TEMPLATE_MODAL"
Length of output: 261
app/client/src/sagas/SaaSPaneSagas.ts (2)
27-27: LGTM!
The import statement is properly placed and follows the project's conventions.
81-96: LGTM! Modal integration looks correct.
The conditional logic properly validates all required conditions before opening the modal.
app/client/src/pages/Editor/Explorer/Pages/AddPageContextMenu.tsx (2)
33-33: LGTM: Clean import addition
The new import aligns with the architectural shift to modal-based page generation.
87-87: Verify modal state management
The onClick handler change looks good, following React best practices. Let's verify the modal state management implementation.
✅ Verification successful
Modal state management implementation is correct
The modal state is properly managed through Redux with appropriate action creators and reducers. The implementation includes:
- Clean action creators for opening/closing modal
- Proper state updates in reducer
- Consistent usage in the GeneratePageModal component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the modal state management implementation
# Check for proper modal state handling and cleanup
# Look for modal state management in reducer
rg -l "SET_GENERATE_PAGE_MODAL_OPEN" --type ts
# Check for proper cleanup in sagas
rg -l "openGeneratePageModal|closeGeneratePageModal" --type ts
Length of output: 767
Script:
#!/bin/bash
# Check the modal state management implementation in reducer
rg "SET_GENERATE_PAGE_MODAL_OPEN" app/client/src/reducers/entityReducers/pageListReducer.tsx -A 5
# Check the action creators and cleanup
rg "openGeneratePageModal|closeGeneratePageModal" app/client/src/pages/Editor/GeneratePage/helpers.ts -A 2
# Check modal state usage in the main component
rg "openGeneratePageModal|closeGeneratePageModal" app/client/src/pages/Editor/GeneratePage/index.tsx -A 2
Length of output: 977
app/client/src/ce/hooks/datasourceEditorHooks.tsx (2)
20-20: LGTM: Import changes align with new modal implementation.
Also applies to: 31-31
99-102: Verify modal implementation consistency.
Let's ensure the modal implementation is consistently used across the application.
app/client/src/reducers/entityReducers/pageListReducer.tsx (1)
342-345: Verify modal state usage across components
Let's verify the integration of these changes with components that previously used the URL-based approach.
✅ Verification successful
Modal state changes are properly integrated
The new modal state implementation is correctly integrated:
- Modal state is managed through Redux with proper open/close actions
- No traces of old URL-based navigation remain
- Component properly uses the state via selectors
- Clean implementation with proper typing and default values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components using the new modal state
rg "isGeneratePageModalOpen" --type ts --type tsx
# Search for any remaining usage of old generateTemplateFormURL
rg "generateTemplateFormURL" --type ts --type tsx
Length of output: 155
Script:
#!/bin/bash
# Search for components using the new modal state without file type restrictions
rg "isGeneratePageModalOpen"
# Search for any remaining usage of old generateTemplateFormURL
rg "generateTemplateFormURL"
# Find all TypeScript/React files that might be related
fd "\.(ts|tsx)$" | grep -i "generate"
Length of output: 3225
Script:
#!/bin/bash
# Let's check the GeneratePage component to see how the modal state is used
cat "app/client/src/pages/Editor/GeneratePage/index.tsx"
# Check the reducer actions related to this modal
rg -A 2 "isGeneratePageModalOpen" "app/client/src/reducers/entityReducers/pageListReducer.tsx"
# Look for any URL-based navigation or old patterns
rg "navigate.*generate" "app/client/src/pages/Editor/GeneratePage"
Length of output: 1900
app/client/src/pages/Editor/index.tsx (2)
54-54: LGTM!
Clean import statement following project conventions.
214-214: LGTM! Verify modal state management.
Modal placement is consistent with other modals in the component.
Let's verify the Redux state management for this modal:
app/client/src/pages/Editor/IntegrationEditor/DatasourceCard.tsx (2)
59-59: LGTM: Clean import addition
The new import follows the existing organization pattern and is correctly placed with other feature imports.
269-272: LGTM: Clean transition to modal-based approach
The change from URL navigation to modal dispatch is implemented correctly, maintaining existing permission checks and analytics tracking.
Let's verify the modal action creator usage:
✅ Verification successful
Modal action creator usage is consistent across the codebase
The openGeneratePageModal action creator is consistently used across the codebase with proper payload structure. All usages follow the same pattern of passing either a datasourceId or no parameters for generic page generation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the openGeneratePageModal action is consistently used across the codebase
rg "openGeneratePageModal" -A 3
Length of output: 5320
app/client/src/sagas/QueryPaneSagas.ts (1)
87-87: LGTM!
The import statement is correctly placed and used in the saga.
app/client/src/ce/constants/ReduxActionConstants.tsx (3)
1073-1078: LGTM! Well-structured action types for generate page feature
The action types follow Redux best practices and maintain consistency with other similar features in the codebase.
1080-1082: LGTM! Error handling action type is properly defined
The error action type follows the established pattern for handling form submission errors.
1291-1291: LGTM! Action types are properly integrated into exports
The new action types are correctly spread into the main ReduxActionTypes and ReduxActionErrorTypes exports.
Also applies to: 1296-1296, 1339-1339
app/client/src/pages/Editor/GeneratePage/components/GeneratePageForm/GeneratePageForm.tsx (1)
613-614: Check the order of dispatching actions
In the goToEditDatasource function, ensure that dispatching setDatasourceViewModeFlag(false) before navigating away doesn't cause any unintended side effects.
Verify that the state update is still relevant after navigation.
app/client/src/pages/Editor/GeneratePage/components/GeneratePageForm/GeneratePageForm.tsx
Show resolved
Hide resolved
ankitakinger
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
|
This PR has increased the number of cyclic dependencies by 2, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (10)
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/Postgres1_Spec.ts (2)
Line range hint
156-156: Remove explicit sleepUsing
agHelper.Sleep(3000)is against the coding guidelines. Replace with proper Cypress wait conditions.-agHelper.Sleep(3000); +cy.waitUntil(() => { + return cy.get('[data-testid="editor-route"]').should('be.visible'); +});
Line range hint
143-149: Add assertions before critical table operationsThe table operations should have proper assertions to ensure the table is in the expected state before reading data.
+cy.get('[data-testid="table-widget"]').should('exist').and('be.visible'); table.ReadTableRowColumnData(0, 1, "v2", 4000).then(($cellData) => { expect($cellData).to.eq(col1Text); });app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/S3_Spec.js (2)
Line range hint
1-250: Test structure needs improvements to align with best practicesThe test file has several areas that need improvement to align with our coding guidelines:
- Replace cy.wait() with better alternatives:
-cy.wait("@getDatasourceStructure") +cy.intercept("GET", "**/getDatasourceStructure").as("getDatasourceStructure") +cy.wait("@getDatasourceStructure").should((interception) => { + expect(interception.response.statusCode).to.eq(200) + expect(interception.response.body).to.have.nested.property("responseMeta.status", 200) +})
- Replace hardcoded waits:
-cy.wait(2000) +cy.get(generatePage.selectTableDropdown).should('be.visible')
- Use data-* attributes for selectors:
-cy.get(".t--test-datasource") +cy.get("[data-cy=test-datasource]")Would you like me to generate a complete refactored version of this test file?
Line range hint
142-146: Add multiple assertions for comprehensive validationThe test uses single assertions which could miss edge cases. Consider adding multiple assertions:
cy.wait("@postExecute").then(({ response }) => { - expect(response.body.data.isExecutionSuccess).to.eq(true); + expect(response.body.data).to.satisfy((data) => { + expect(data.isExecutionSuccess).to.eq(true) + expect(data.body).to.not.be.null + expect(data.statusCode).to.eq(200) + return true + }) });app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/MySQL1_Spec.ts (2)
Line range hint
43-73: Replace cy.wait and sleep with better alternativesThe test contains anti-patterns that should be avoided:
- Usage of
cy.wait("@getDatasourceStructure")and other network waits- Usage of
agHelper.Sleep(3000)Consider using Cypress's built-in retry-ability:
- assertHelper.AssertNetworkStatus("@getDatasourceStructure"); + cy.intercept('GET', '**/datasourceStructure').as('getDatasourceStructure'); + cy.wait('@getDatasourceStructure').its('response.statusCode').should('eq', 200); - agHelper.Sleep(3000); + agHelper.WaitUntilEleAppear(locators._yourTargetElement);
Line range hint
359-363: Refactor test cleanup strategyThe use of
afterhook for cleanup is against the coding guidelines. Consider moving the cleanup logic into the test cases themselves.- after("Verify Deletion of the datasource when Pages/Actions associated are not removed yet", - () => { - dataSources.DeleteDatasourceFromWithinDS(dsName, 409); - }, - ); + it("11. Cleanup: Verify Deletion of the datasource", () => { + dataSources.DeleteDatasourceFromWithinDS(dsName, 409); + });app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/MySQL2_Spec.ts (4)
402-402: Simplify page generation logicThe function has been modified to use a more direct approach for page generation, which is good. However, it still contains redundant network status checks.
Consider consolidating the network status checks:
-assertHelper.AssertNetworkStatus("@replaceLayoutWithCRUDPage", 201); -//assertHelper.AssertNetworkStatus("@getActions", 200);//Since failing sometimes -assertHelper.AssertNetworkStatus("@postExecute", 200); -assertHelper.AssertNetworkStatus("@updateLayout", 200); +const expectedResponses = { + "@replaceLayoutWithCRUDPage": 201, + "@postExecute": 200, + "@updateLayout": 200 +}; +Object.entries(expectedResponses).forEach(([route, status]) => { + assertHelper.AssertNetworkStatus(route, status); +});
Line range hint
8-8: Use cy.wait for network requests instead of agHelper.SleepMultiple instances of
agHelper.Sleep()are used throughout the test file. This is against Cypress best practices.Replace sleep calls with proper network request waiting:
-agHelper.Sleep(2000); //for update to reflect! +cy.wait('@postExecute').its('response.statusCode').should('eq', 200);Also applies to: 9-9
Line range hint
12-24: Improve test data managementThe test data setup is hardcoded in the test file. This makes it difficult to maintain and update.
Consider moving the test data to a separate fixture file and using Cypress's fixture functionality:
// fixtures/stores-data.json { "createTableQuery": "CREATE TABLE Stores...", "testData": [ { "store_id": 2106, "name": "Hillstreet News and Tobacco", // ... } ] } // In test file cy.fixture('stores-data').then((data) => { dataSources.CreateQueryForDS(dsName, data.createTableQuery, "CreateStores"); });
Line range hint
168-171: Skip test with proper documentationThe skipped test case needs better documentation about the issue.
Add more context to the skip message:
-it.skip("7. Verify Refresh table from Deploy page - on Stores & verify all updates persists : Skipped till #29870 gets fixed", () => { +it.skip("7. Verify Refresh table from Deploy page - on Stores & verify all updates persists", () => { + // Skipped due to issue #29870: Table refresh not updating view in deployment mode + // TODO: Re-enable once the following is fixed: + // 1. Table refresh mechanism in deployment mode + // 2. Row selection after refresh
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/MySQL1_Spec.ts(3 hunks)app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/MySQL2_Spec.ts(1 hunks)app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/Postgres1_Spec.ts(3 hunks)app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/S3_Spec.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/MySQL1_Spec.ts (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/Regression/ServerSide/GenerateCRUD/MySQL2_Spec.ts (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/Regression/ServerSide/GenerateCRUD/Postgres1_Spec.ts (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/Regression/ServerSide/GenerateCRUD/S3_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.
🔇 Additional comments (2)
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/S3_Spec.js (1)
55-55: LGTM: Text assertion update is correct
The assertion text has been correctly updated to match the new UI text "Generate page from data".
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/MySQL1_Spec.ts (1)
50-50: LGTM: Button click implementations are consistent
The button click implementations for page generation are consistent with the modal-based approach and follow the data-attribute selector guidelines.
Also applies to: 97-97, 115-115
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/Postgres1_Spec.ts
Show resolved
Hide resolved
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11887097401. |
|
Deploy-Preview-URL: https://ce-37414.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11891129363. |
|
This PR has increased the number of cyclic dependencies by 2, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
app/client/src/pages/Editor/GeneratePage/index.tsx (3)
1-18: Consider adding type imports for modal eventsAdd types for modal event handlers to improve type safety and documentation.
+ import { ModalOpenChangeHandler } from "@appsmith/ads";
19-22: Add explicit return type to the componentThe component should have an explicit return type for better type safety.
- function GeneratePageModal() { + function GeneratePageModal(): JSX.Element {
35-45: Consider moving inline styles to theme constantsThe hardcoded width in inline styles could be moved to a theme constant or styled-component for better maintainability.
- <ModalContent style={{ width: "444px" }}> + <ModalContent className="generate-page-modal">Then define the width in your theme or styled-components:
const StyledModalContent = styled(ModalContent)` &.generate-page-modal { width: ${({ theme }) => theme.modalSizes.medium}px; } `;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/S3_Spec.js(1 hunks)app/client/src/ce/constants/messages.ts(1 hunks)app/client/src/pages/Editor/GeneratePage/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/cypress/e2e/Regression/ServerSide/GenerateCRUD/S3_Spec.js
- app/client/src/ce/constants/messages.ts
🔇 Additional comments (2)
app/client/src/pages/Editor/GeneratePage/index.tsx (2)
23-32: LGTM! Well-implemented event handler
The event handler is properly memoized with useCallback and has correct dependencies.
49-49: LGTM! Clean export statement
The default export is properly defined.
|
Deploy-Preview-URL: https://ce-37414.dp.appsmith.com |
ankitakinger
changed the title
github-actions
bot
added
skip-changelog
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
app/client/src/pages/Editor/GeneratePage/components/GeneratePageForm/GoogleSheetForm.tsx (1)
Line range hint
449-449: Fix useCallback dependency arrayThe dependency array for debouncedFetchColumns includes
selectedSheettwice and is missingselectedSpreadsheet. This could cause unnecessary re-renders or stale closures.}, [ selectedSheet, selectedDatasource, - selectedSheet + selectedSpreadsheet ]);app/client/src/pages/Editor/GeneratePage/components/GeneratePageForm/GeneratePageForm.tsx (2)
672-912: Enhance form accessibilityThe form components should include proper ARIA attributes for better accessibility:
- Add
aria-labelto form controls- Include
aria-invalidfor error states- Provide better error announcements
<Select data-testid="t--datasource-dropdown" + aria-label="Select datasource" + aria-invalid={!!tableDropdownErrorMsg} getPopupContainer={(triggerNode) => triggerNode.parentNode.parentNode} // ... rest of the props />
789-793: Enhance error state feedbackConsider adding more descriptive error messages and visual feedback:
- Add error icons
- Include suggested actions for resolution
- Provide clear next steps
{tableDropdownErrorMsg && ( - <ErrorMsg className="ads-dropdown-errorMsg"> + <ErrorMsg className="ads-dropdown-errorMsg" role="alert"> + <Icon name="error" size="small" /> {tableDropdownErrorMsg} + {fetchingDatasourceConfigError && ( + <Text>Please check your connection settings and try again.</Text> + )} </ErrorMsg> )}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/client/src/pages/Editor/GeneratePage/components/GeneratePageForm/GeneratePageForm.tsx(9 hunks)app/client/src/pages/Editor/GeneratePage/components/GeneratePageForm/GoogleSheetForm.tsx(1 hunks)
🔇 Additional comments (3)
app/client/src/pages/Editor/GeneratePage/components/GeneratePageForm/GoogleSheetForm.tsx (1)
304-306: Consider using a more robust parent container selector
The double parentNode access (triggerNode.parentNode.parentNode) could be fragile if the DOM structure changes. Consider using a more specific selector or a ref to ensure stable dropdown positioning.
Consider using a dedicated container ref:
function GoogleSheetForm(props: Props) {
+ const containerRef = useRef<HTMLDivElement>(null);
// ... existing code ...
return (
- <>
+ <div ref={containerRef}>
{selectedSpreadsheet.value ? (
<SelectWrapper width={DROPDOWN_DIMENSION.WIDTH}>
<Select
getPopupContainer={() => containerRef.current || document.body}
// ... other props
>app/client/src/pages/Editor/GeneratePage/components/GeneratePageForm/GeneratePageForm.tsx (2)
14-17: LGTM: Modal-related imports are properly organized
The new imports for modal management are well-structured and follow the established pattern.
Also applies to: 21-24
599-599: Consider handling modal close after state updates
The modal is closed immediately after dispatching the generate action. Consider waiting for the action to complete before closing the modal to prevent potential race conditions.
const handleFormSubmit = () => {
const payload = {
columns: [],
searchColumn: selectedColumn.value,
tableName: selectedTable.value || "",
};
- generatePageAction(payload);
- dispatch(closeGeneratePageModal());
+ generatePageAction(payload).finally(() => {
+ dispatch(closeGeneratePageModal());
+ });
};
|
Deploy-Preview-URL: https://ce-37414.dp.appsmith.com |
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11897698644. |
|
Deploy-Preview-URL: https://ce-37414.dp.appsmith.com |
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
app/client/src/pages/Editor/GeneratePage/components/GeneratePageForm/GeneratePageForm.tsx (3)
Line range hint
451-464: Remove duplicate dependency in useCallbackThe
generateCRUDSupportedPluginappears twice in the dependency array of useCallback, which is unnecessary.[ generateCRUDSupportedPlugin, selectDataSource, setSelectedDatasourceTableOptions, setSelectedTableColumnOptions, selectTable, selectColumn, dispatch, setSelectedDatasourceIsInvalid, selectedDatasource, - generateCRUDSupportedPlugin, ],
678-687: Improve readability with early returnsThe nested if-else structure could be simplified using early returns for better readability.
- onChange={(value) => { - if (value === CONNECT_NEW_DATASOURCE_OPTION_ID) { - routeToCreateNewDatasource(); - } else { - onSelectDataSource( - value, - dataSourceOptions.find((ds) => ds.value === value), - ); - } - }} + onChange={(value) => { + if (value === CONNECT_NEW_DATASOURCE_OPTION_ID) { + return routeToCreateNewDatasource(); + } + onSelectDataSource( + value, + dataSourceOptions.find((ds) => ds.value === value), + ); + }}
675-731: Consider extracting Select component logicThere are multiple Select components with similar patterns and props. Consider extracting this into a reusable component to reduce code duplication and improve maintainability.
Example structure:
interface CommonSelectProps { testId: string; options: DropdownOption[]; value: DropdownOption; onChange: (value: string, option: DropdownOption) => void; isDisabled?: boolean; isLoading?: boolean; } function CommonSelect(props: CommonSelectProps) { // Common select implementation }Also applies to: 741-787, 816-868
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/client/src/pages/Editor/GeneratePage/components/GeneratePageForm/GeneratePageForm.tsx(9 hunks)app/client/src/pages/Editor/GeneratePage/components/GeneratePageForm/GoogleSheetForm.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/pages/Editor/GeneratePage/components/GeneratePageForm/GoogleSheetForm.tsx
🔇 Additional comments (1)
app/client/src/pages/Editor/GeneratePage/components/GeneratePageForm/GeneratePageForm.tsx (1)
563-563: Verify modal closing behavior
The modal is closed in multiple places. Verify that there are no edge cases where the modal might remain open, especially during error scenarios.
Also applies to: 588-588, 614-614
✅ Verification successful
Let me search for the modal state management and error handling to ensure comprehensive verification.
Let me check for the form submission and error handling flow to complete the verification.
Modal closing implementation is properly handled
The modal closing behavior is well-implemented with proper state management:
- Closes on successful form submission
- Closes on navigation to datasource creation
- Has dedicated action types for success and error scenarios
- Uses Redux for consistent state management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all modal close actions to ensure consistent handling
rg "closeGeneratePageModal|setDatasourceViewModeFlag" -A 2 -B 2
Length of output: 10710
Script:
#!/bin/bash
# Search for modal state management and related error handling
rg "SET_GENERATE_PAGE_MODAL|generatePageError|GeneratePageError" -A 2 -B 2
# Search for any error handlers in the GeneratePageForm component
ast-grep --pattern 'catch ($_) {
$$$
}'
Length of output: 2587
Script:
#!/bin/bash
# Search for form submission and error handling
rg "SUBMIT_GENERATE_PAGE_FORM" -A 5 -B 5
# Search for any error handling sagas
ast-grep --pattern 'function* $name($_) {
try {
$$$
} catch($_) {
$$$
}
}'
Length of output: 1509
app/client/src/pages/Editor/GeneratePage/components/GeneratePageForm/GeneratePageForm.tsx
Show resolved
Hide resolved
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11899143299. |
|
Deploy-Preview-URL: https://ce-37414.dp.appsmith.com |
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/client/src/pages/Editor/GeneratePage/components/GeneratePageForm/GeneratePageForm.tsx (2)
Line range hint
532-546: Consider adding type safety for modal parametersThe parameter handling logic is correct, but could benefit from type safety.
- if (params?.datasourceId || params?.new_page) { + interface ModalParams { + datasourceId?: string; + new_page?: boolean; + } + if ((params as ModalParams)?.datasourceId || (params as ModalParams)?.new_page) {
563-563: Add error handling for modal closureThe modal is correctly closed after actions, but consider adding error handling to ensure the modal closes even if the action fails.
- dispatch(generateTemplateToUpdatePage(payload)); - dispatch(closeGeneratePageModal()); + try { + dispatch(generateTemplateToUpdatePage(payload)); + } finally { + dispatch(closeGeneratePageModal()); + }Also applies to: 589-589, 613-614
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/src/pages/Editor/GeneratePage/components/GeneratePageForm/GeneratePageForm.tsx(9 hunks)
🔇 Additional comments (2)
app/client/src/pages/Editor/GeneratePage/components/GeneratePageForm/GeneratePageForm.tsx (2)
14-17: LGTM: Modal-related imports and selectors
The new imports align well with the modal-based interaction pattern mentioned in the PR objectives.
Also applies to: 21-24
703-704: Investigate and fix virtual scrolling issues with Cypress tests
Multiple instances of disabled virtual scrolling indicate a systemic issue that needs proper investigation.
Consider:
- Investigating why virtual scrolling breaks Cypress tests
- Implementing a proper fix that works with Cypress
- Removing all temporary workarounds
Also applies to: 770-771
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11907272123. |
|
Deploy-Preview-URL: https://ce-37414.dp.appsmith.com |
…wing the IDE 2.0 interaction pattern (appsmithorg#37414) ## Description Updating generate page interaction to show it in a modal following the IDE 2.0 interaction pattern Fixes [appsmithorg#32952](appsmithorg#32952) ## 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/11900113834> > Commit: 3903c44 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11900113834&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Mon, 18 Nov 2024 21:26:44 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Introduced a modal for generating pages, enhancing user interaction. - Added new action types and constants for managing page generation processes. - Updated UI messages for clarity in the page generation context. - Improved handling of datasource selection and page generation in various components. - **Bug Fixes** - Improved error handling in various components to prevent silent failures. - **Refactor** - Streamlined routing logic by removing deprecated paths and functions. - Transitioned from direct navigation to modal-based interactions for page generation. - Enhanced control flow and error handling within components. - **Chores** - Updated import paths for better organization of action-related functions within the Redux architecture. - **Tests** - Enhanced test cases for CRUD operations, ensuring better validation and error handling. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Updating generate page interaction to show it in a modal following the IDE 2.0 interaction pattern
Fixes #32952
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/11900113834
Commit: 3903c44
Cypress dashboard.
Tags:
@tag.AllSpec:
Mon, 18 Nov 2024 21:26:44 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Chores
Tests