fix: auto redirect from datasource page to module editor #35164
Conversation
WalkthroughThe recent changes enhance state management and saga handling in the application, particularly regarding datasource operations. A new selector, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant APIEditor
participant DatasourceConfig
User->>APIEditor: Click on Save Datasource
APIEditor->>DatasourceConfig: Redirect to datasource config page
User->>DatasourceConfig: Save datasource
DatasourceConfig->>APIEditor: Redirect to correct API editor page
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
github-actions
bot
added
Bug
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10087095676. |
![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, codebase verification and nitpick comments (1)
app/client/src/ce/selectors/modulesSelector.ts (1)
9-9: EnsuregetCurrentModuleIdreturns the correct module ID.The function currently returns an empty string. Ensure that the correct module ID is retrieved from the application state.
- Location:
app/client/src/ce/selectors/modulesSelector.tsat line 9- Usage:
app/client/src/sagas/DatasourcesSagas.tsat line 9 and 14Analysis chain
Ensure
getCurrentModuleIdreturns the correct module ID.The function currently returns an empty string. Ensure that the correct module ID is retrieved from the application state.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage and implementation of `getCurrentModuleId`. # Test: Search for the function usage. Expect: Function should retrieve the correct module ID from the state. rg --type typescript -A 5 $'getCurrentModuleId'Length of output: 81
Script:
#!/bin/bash # Description: Verify the usage and implementation of `getCurrentModuleId`. # Test: Search for the function usage. Expect: Function should retrieve the correct module ID from the state. rg --type ts -A 5 $'getCurrentModuleId'Length of output: 1169
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- app/client/src/ce/selectors/modulesSelector.ts (1 hunks)
- app/client/src/reducers/uiReducers/datasourcePaneReducer.ts (2 hunks)
- app/client/src/sagas/ApiPaneSagas.ts (3 hunks)
- app/client/src/sagas/DatasourcesSagas.ts (5 hunks)
- app/client/src/sagas/tests/ApiPaneSagas.test.ts (2 hunks)
Additional comments not posted (11)
app/client/src/reducers/uiReducers/datasourcePaneReducer.ts (2)
63-63: Simplify the type ofactionparameter.The type of the
actionparameter in theSTORE_AS_DATASOURCE_UPDATEcase has been simplified. This change enhances type safety and maintainability.
26-26: EnsureparentEntityIdis correctly populated.The
parentEntityIdfield replacespageId. Ensure that this field is correctly populated and used throughout the application.Verification successful
Let's correct the previous script to accurately search for the usage of
parentEntityIdacross all TypeScript files.
Ensure
parentEntityIdis correctly populated.The
parentEntityIdfield is indeed used and populated throughout the application as observed in multiple instances across the codebase.
- Examples of usage:
apiEditorIdURL({ parentEntityId: contextId, apiId: response.data.id })parentEntityId: pageId || moduleIdparentEntityId: actionRouteInfo.parentEntityId || ""const { parentEntityId, parentEntityKey } = resolveParentEntityMetadata(...)parentEntityId: action.payload.destinationPageIdGiven these instances, it can be confirmed that
parentEntityIdis correctly populated and used throughout the application.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage and population of `parentEntityId`. # Test: Search for the usage of `parentEntityId`. Expect: Field should be correctly populated and used throughout the application. rg --type typescript -A 5 $'parentEntityId'Length of output: 77
Script:
#!/bin/bash # Description: Verify the usage and population of `parentEntityId`. # Find all TypeScript files and search for the usage of `parentEntityId`. fd -e ts -x rg 'parentEntityId' {}Length of output: 1597
app/client/src/sagas/__tests__/ApiPaneSagas.test.ts (2)
1-12: Good practice: Import necessary dependencies.The import statements include necessary dependencies for the new test suite. This is a good practice to ensure the test functions properly.
64-140: Comprehensive test forhandleDatasourceCreatedSaga.The new test suite for
handleDatasourceCreatedSagais comprehensive and verifies the correct behavior of the saga. Ensure that all edge cases are covered.app/client/src/sagas/ApiPaneSagas.ts (3)
590-590: Good job on exporting the function!Exporting
handleDatasourceCreatedSagaimproves its usability within the application.
611-612: Great improvement on type safety!Updating the type of
actionRouteInfotoReturnType<typeof getDatasourceActionRouteInfo>ensures more accurate type representation.
648-649: Ensure the new logic forpageIdis correct.The logic for determining
pageIdhas been updated to useactionRouteInfo?.parentEntityId. Verify that this change aligns with the new data structure and requirements.Verification successful
Let's correct the file type and re-run the search to verify the usage of
parentEntityIdin the codebase.
The new logic for
pageIdis correct.The usage of
parentEntityIdis consistent across the codebase, indicating that it is an integral part of the data structure. The change to useactionRouteInfo?.parentEntityIdaligns with this structure and is appropriate.
- Files and lines to review:
app/client/src/sagas/ApiPaneSagas.ts(Lines 648-649)Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `parentEntityId` in the codebase to ensure consistency with the new data structure. # Test: Search for `parentEntityId` usage. Expect: Consistent usage across the codebase. rg --type typescript -A 5 $'parentEntityId'Length of output: 77
Script:
#!/bin/bash # Description: Verify the usage of `parentEntityId` in the codebase to ensure consistency with the new data structure. # Test: Search for `parentEntityId` usage. Expect: Consistent usage across the codebase. rg --type ts -A 5 'parentEntityId'Length of output: 54483
app/client/src/sagas/DatasourcesSagas.ts (4)
1066-1067: Great improvement on type safety!Updating the type of
actionRouteInfotoReturnType<typeof getDatasourceActionRouteInfo>ensures more accurate type representation.
1305-1305: Good addition of moduleId!Adding
moduleIdenhances the flexibility of the action by allowing it to reference the current module's ID.
1405-1405: Ensure the new logic for parentEntityId is correct.Using
parentEntityIdinstead ofpageIdwhen pushing to history ensures the correct identifier is utilized based on the context. Verify that this change aligns with the new data structure and requirements.Verification successful
Let's correct the file type and re-run the search to verify the usage of
parentEntityId.
Ensure the new logic for parentEntityId is correct.
The usage of
parentEntityIdis consistent across the codebase, aligning with the new data structure and requirements. The change ensures that the correct identifier is utilized based on the context.
- Verified instances in:
app/client/src/sagas/CurlImportSagas.tsapp/client/src/sagas/ActionSagas.tsapp/client/src/reducers/uiReducers/datasourcePaneReducer.tsapp/client/src/sagas/ApiPaneSagas.tsapp/client/src/pages/common/datasourceAuth/index.tsx- And many other files.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `parentEntityId` in the codebase to ensure consistency with the new data structure. # Test: Search for `parentEntityId` usage. Expect: Consistent usage across the codebase. rg --type typescript -A 5 $'parentEntityId'Length of output: 77
Script:
#!/bin/bash # Description: Verify the usage of `parentEntityId` in the codebase to ensure consistency with the new data structure. # Test: Search for `parentEntityId` usage. Expect: Consistent usage across the codebase. rg --type ts -A 5 $'parentEntityId'Length of output: 54483
1363-1363: Ensure the new logic for parentEntityId is correct.Including
parentEntityIdin the action payload, which can be eitherpageIdormoduleId, improves flexibility. Verify that this change aligns with the new data structure and requirements.Verification successful
Let's correct the file type and re-run the search to ensure we gather the necessary information about the usage of
parentEntityIdacross the codebase.
Ensure the new logic for parentEntityId is correct.
The new logic for
parentEntityId(i.e.,pageId || moduleId) appears to be consistently used across the codebase. The change aligns with the existing data structure and requirements, as evidenced by multiple instances whereparentEntityIdis correctly assigned and utilized.
- Files with relevant changes:
app/client/src/sagas/DatasourcesSagas.tsapp/client/src/sagas/ApiPaneSagas.tsapp/client/src/pages/common/datasourceAuth/index.tsxapp/client/src/pages/Editor/gitSync/ReconnectDatasourceModal.tsxapp/client/src/pages/Editor/IntegrationEditor/NewApi.tsxapp/client/src/pages/Editor/IntegrationEditor/NewQuery.tsxapp/client/src/pages/Editor/IntegrationEditor/DatasourceHome.tsxapp/client/src/pages/Editor/Explorer/Files/index.tsxapp/client/src/sagas/ActionSagas.tsapp/client/src/sagas/helpers.tsThe new logic for setting
parentEntityIdaspageId || moduleIdis correctly integrated and should work as expected within the updated data structure.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `parentEntityId` in the codebase to ensure consistency with the new data structure. # Test: Search for `parentEntityId` usage. Expect: Consistent usage across the codebase. rg --type typescript -A 5 $'parentEntityId'Length of output: 77
Script:
#!/bin/bash # Description: Verify the usage of `parentEntityId` in the codebase to ensure consistency with the new data structure. # Test: Search for `parentEntityId` usage. Expect: Consistent usage across the codebase. rg --type ts -A 5 'parentEntityId'Length of output: 54483
|
Deploy-Preview-URL: https://ce-35164.dp.appsmith.com |
|
EE DP -> https://ee-4742.dp.appsmith.com/ |
|
@ashit-rath redirection not working in module editor , |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/src/sagas/ApiPaneSagas.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/sagas/ApiPaneSagas.ts
|
Looks good @ashit-rath |
| @@ -1305,6 +1302,7 @@ function* storeAsDatasourceSaga() { | |||
| const { values } = yield select(getFormData, API_EDITOR_FORM_NAME); | |||
| const applicationId: string = yield select(getCurrentApplicationId); | |||
| const pageId: string | undefined = yield select(getCurrentPageId); | |||
| const moduleId: string | undefined = yield select(getCurrentModuleId); | |||
There was a problem hiding this comment.
Is it possible to get the parentEntityId from the URL instead of doing this check? If not, then should we add TODO to add workflows handling here?
There was a problem hiding this comment.
Any reason for grabbing the id from URL? Also what do you expect for workflows? since it does not have a parentEntityId but only the editorId which is the workflowId
There was a problem hiding this comment.
Just checked, seems like not needed.
| jest.resetAllMocks(); | ||
| }); | ||
|
|
||
| it("should pass parentEntityId to apiEditorIdURL and redirect to correct url when in app", async () => { |
There was a problem hiding this comment.
Should we also add tests for scenarios in workflows and packages. Not urgent, can park as TODO or github issue also.
There was a problem hiding this comment.
Yes, we have to take care of this in EE as the URLAssembly rules are not present here. I'll create an issue
There was a problem hiding this comment.
Oh yeah, makes sense. I confused this with EE.
| @@ -1305,6 +1302,7 @@ function* storeAsDatasourceSaga() { | |||
| const { values } = yield select(getFormData, API_EDITOR_FORM_NAME); | |||
| const applicationId: string = yield select(getCurrentApplicationId); | |||
| const pageId: string | undefined = yield select(getCurrentPageId); | |||
| const moduleId: string | undefined = yield select(getCurrentModuleId); | |||
There was a problem hiding this comment.
Just checked, seems like not needed.
| jest.resetAllMocks(); | ||
| }); | ||
|
|
||
| it("should pass parentEntityId to apiEditorIdURL and redirect to correct url when in app", async () => { |
There was a problem hiding this comment.
Oh yeah, makes sense. I confused this with EE.
…#35164) ## Description When create datasource is initiated from the API editor by clicking "Save as datasource", it fixes the redirection back to the API editor when `Save` is clicked for modules in package editor. This PR also refactors the usage of typing of `actionRouteInfo` and introduces `parentEntityId` instead of using `pageId`, `moduleId`, `workflowId` separately. This fix also verifies the sanity of 1. API in apps 2. API in workflows Fixes appsmithorg#31506 ## Automation /ok-to-test tags="@tag.Datasource,@tag.Sanity" ### 🔍 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/10093685845> > Commit: 785cbe3 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10093685845&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Datasource,@tag.Sanity` > Spec: > <hr>Thu, 25 Jul 2024 12:39:50 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** - Introduced a new function to retrieve the current module ID, enhancing state management. - Added support for handling `parentEntityId` in various data source sagas. - **Bug Fixes** - Improved type safety for action handling in sagas. - **Tests** - Added a new test suite for the `handleDatasourceCreatedSaga` to validate its behavior upon datasource creation. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
When create datasource is initiated from the API editor by clicking "Save as datasource", it fixes the redirection back to the API editor when
Saveis clicked for modules in package editor.This PR also refactors the usage of typing of
actionRouteInfoand introducesparentEntityIdinstead of usingpageId,moduleId,workflowIdseparately.This fix also verifies the sanity of
Fixes #31506
Automation
/ok-to-test tags="@tag.Datasource,@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10093685845
Commit: 785cbe3
Cypress dashboard.
Tags:
@tag.Datasource,@tag.SanitySpec:
Thu, 25 Jul 2024 12:39:50 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
parentEntityIdin various data source sagas.Bug Fixes
Tests
handleDatasourceCreatedSagato validate its behavior upon datasource creation.