Chore/generate page modal #37414
Chore/generate page modal #37414
Conversation
WalkthroughThe pull request includes significant modifications across multiple files, primarily focusing on the removal of the Changes
Suggested labels
Suggested reviewers
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.
| value, | ||
| dataSourceOptions.find((ds) => ds.value === value), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure dataSourceOptions.find does not return undefined
In the onSelectDataSource function, verify that dataSourceOptions.find returns a valid DropdownOption before using it to prevent potential runtime errors.
Apply this diff to add a safety check:
onSelectDataSource(
value,
- dataSourceOptions.find((ds) => ds.value === value),
+ dataSourceOptions.find((ds) => ds.value === value) || DEFAULT_DROPDOWN_OPTION,
);Committable suggestion skipped: line range outside the PR's diff.
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
| @@ -38,6 +38,7 @@ describe( | |||
| assertHelper.AssertNetworkStatus("@getDatasourceStructure"); //Making sure table dropdown is populated | |||
| agHelper.GetNClick(dataSources._selectTableDropdown, 0, true); | |||
| agHelper.GetNClickByContains(dataSources._dropdownOption, "film"); | |||
| agHelper.GetNClick(dataSources._generatePageBtn); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use data- attributes for selectors*
The selectors _generatePageBtn and _datasourceCardGeneratePageBtn should use data-* attributes for better maintainability and reliability. Additionally, add assertions to verify element state before clicking.
-agHelper.GetNClick(dataSources._generatePageBtn);
+cy.get('[data-testid="generate-page-button"]')
+ .should('be.visible')
+ .should('be.enabled')
+ .click();Also applies to: 90-90, 109-109
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
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
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11861857120
Commit: d9a64cc
Cypress dashboard.
Tags:
@tag.AllSpec:
Fri, 15 Nov 2024 21:18:12 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
renderKVArrayfunction to prevent silent failures.UI/UX Changes
Refactor