removed redundant api call#33911
removed redundant api call#33911saiprabhu-dandanayak wants to merge 4 commits intoappsmithorg:releasefrom
Conversation
WalkthroughThe updates introduce new configurations for Cypress video recording, enhance the functionality of template handling in the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant Cypress
participant TemplatesApi
participant ReduxStore
User->>Cypress: Run tests
Cypress->>TemplatesApi: Fetch template information
TemplatesApi->>ReduxStore: Check for template in store
alt Template in store
ReduxStore-->>TemplatesApi: Return template data
else Template not in store
TemplatesApi-->>TemplatesApi: Fetch from API
end
TemplatesApi-->>Cypress: Return template data
Cypress-->>User: Display test results
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 Configration File (
|
saiprabhu-dandanayak
changed the title
![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 (1)
app/client/src/api/TemplatesApi.ts (1)
Line range hint
79-143: Consider refactoring the class to a namespace since it contains only static members.Tools
Biome
[error] 96-96: Unexpected any. Specify a different type.
[error] 99-99: Template literals are preferred over string concatenation.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
app/client/src/api/TemplatesApi.ts (2)
Line range hint
79-141: Class contains only static members.Consider converting
TemplatesAPIinto a namespace or a module with exported functions, as using a class here does not leverage any OOP features like instances or state.Tools
Biome
[error] 95-95: Unexpected any. Specify a different type.
[error] 97-97: Template literals are preferred over string concatenation.
Line range hint
86-86: Use template literals for string concatenation.Using template literals can make the code cleaner and easier to read. Here's an example refactor for one of the methods:
- return Api.get(TemplatesAPI.baseUrl + `/app-templates/${templateId}`); + return Api.get(`${TemplatesAPI.baseUrl}/app-templates/${templateId}`);Apply similar changes to other methods where string concatenation is used.
Also applies to: 97-97, 104-104, 112-113, 123-124, 131-131, 137-137
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/src/api/TemplatesApi.ts (5 hunks)
Additional context used
Biome
app/client/src/api/TemplatesApi.ts
[error] 79-141: Avoid classes that contain only static members.
[error] 86-86: Template literals are preferred over string concatenation.
[error] 86-86: Do not use template literals if interpolation and special-character handling are not needed.
[error] 95-95: Unexpected any. Specify a different type.
[error] 97-97: Template literals are preferred over string concatenation.
[error] 104-104: Template literals are preferred over string concatenation.
[error] 112-113: Template literals are preferred over string concatenation.
[error] 123-124: Template literals are preferred over string concatenation.
[error] 131-131: Template literals are preferred over string concatenation.
[error] 131-131: Do not use template literals if interpolation and special-character handling are not needed.
[error] 137-137: Template literals are preferred over string concatenation.
[error] 137-137: Do not use template literals if interpolation and special-character handling are not needed.
Additional comments not posted (3)
app/client/src/api/TemplatesApi.ts (3)
32-32: Change in type definition fromFetchTemplatesResponsetoTemplatesResponse.Ensure that all usages of
FetchTemplatesResponseare updated toTemplatesResponsewhere applicable.
69-78: Addition ofcreateTemplateResponsefunction.This function standardizes the response format for template data, which enhances maintainability and consistency across API responses.
90-96: Refactor to fetch template information from Redux store before making an API call.This change optimizes performance by reducing unnecessary API calls if the data is already available in the store.
Verification successful
The search results indicate that template-related actions are being dispatched, and the
TemplatesApiis being imported and used in various files. This suggests that the Redux store is being populated with template data before the function inTemplatesApi.tsis called.
app/client/src/pages/Templates/TemplateView.tsx: Dispatches actions likegetTemplateInformationandgetSimilarTemplatesInit.app/client/src/pages/Templates/ForkTemplate.tsx: DispatchesimportTemplateToWorkspace.app/client/src/ce/pages/Applications/CreateNewAppFromTemplateModal/index.tsx: DispatchessetActiveLoadingTemplateIdandimportTemplateToWorkspace.These actions ensure that the Redux store is populated with the necessary template data before the function in
TemplatesApi.tsis called.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the Redux store is properly populated before this function is called. # Test: Search for Redux store population logic. Expect: Proper handling and population logic before this function is called. rg --type python $'store.dispatch'Length of output: 64
Script:
#!/bin/bash # Description: Verify that the Redux store is properly populated before this function is called. # Test: Search for Redux store population logic. Expect: Proper handling and population logic before this function is called. rg --type js 'store.dispatch' rg --type ts 'store.dispatch'Length of output: 2416
Script:
#!/bin/bash # Description: Verify that template-related actions are being dispatched to populate the Redux store. # Test: Search for template-related actions being dispatched. Expect: Actions related to templates being dispatched. rg --type js 'dispatch.*template' rg --type ts 'dispatch.*template'Length of output: 679
Script:
#!/bin/bash # Description: Verify that the function in TemplatesApi.ts is called after template-related actions are dispatched. # Test: Search for the function call in TemplatesApi.ts. Expect: Function is called after template-related actions. rg --type ts 'TemplatesApi' --context 10Length of output: 30695
Tools
Biome
[error] 95-95: Unexpected any. Specify a different type.
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (2)
app/client/src/api/TemplatesApi.ts (2)
Line range hint
86-86: Use template literals for string concatenation.- return Api.get(TemplatesAPI.baseUrl + `/app-templates`); + return Api.get(`${TemplatesAPI.baseUrl}/app-templates`); - return Api.get(TemplatesAPI.baseUrl + `/app-templates/${templateId}`); + return Api.get(`${TemplatesAPI.baseUrl}/app-templates/${templateId}`); - return Api.get(TemplatesAPI.baseUrl + `/app-templates/${templateId}/similar`); + return Api.get(`${TemplatesAPI.baseUrl}/app-templates/${templateId}/similar`); - return Api.post(TemplatesAPI.baseUrl + `/app-templates/${templateId}/import/${workspaceId}`); + return Api.post(`${TemplatesAPI.baseUrl}/app-templates/${templateId}/import/${workspaceId}`); - return Api.post(TemplatesAPI.baseUrl + `/app-templates/${templateId}/merge/${applicationId}/${organizationId}`, body); + return Api.post(`${TemplatesAPI.baseUrl}/app-templates/${templateId}/merge/${applicationId}/${organizationId}`, body); - return Api.get(TemplatesAPI.baseUrl + `/app-templates/filters`); + return Api.get(`${TemplatesAPI.baseUrl}/app-templates/filters`); - return Api.post(TemplatesAPI.baseUrl + `/app-templates/publish/community-template`, body); + return Api.post(`${TemplatesAPI.baseUrl}/app-templates/publish/community-template`, body);This change replaces string concatenation with template literals, which is a more modern and readable approach in JavaScript.
Also applies to: 97-97, 104-104, 112-113, 123-124, 131-131, 137-137
Line range hint
79-141: Consider refactoring theTemplatesAPIclass.The
TemplatesAPIclass contains only static methods and could be refactored into a simpler, functional module. This would reduce complexity and improve modularity.Tools
Biome
[error] 97-97: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/src/api/TemplatesApi.ts (5 hunks)
Additional context used
Biome
app/client/src/api/TemplatesApi.ts
[error] 79-141: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
[error] 86-86: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
[error] 86-86: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
[error] 97-97: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
[error] 104-104: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
[error] 112-113: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
[error] 123-124: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
[error] 131-131: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
[error] 131-131: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
[error] 137-137: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
[error] 137-137: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (2)
app/client/src/api/TemplatesApi.ts (2)
Line range hint
74-136: Consider refactoring theTemplatesAPIclass.The
TemplatesAPIclass contains only static methods. It might be more appropriate to use a simple object or separate functions instead of a class to hold these static methods. This can improve modularity and potentially reduce overhead.Tools
Biome
[error] 92-92: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
Line range hint
81-81: Use template literals for string concatenation.- return Api.get(TemplatesAPI.baseUrl + `/app-templates`); + return Api.get(`${TemplatesAPI.baseUrl}/app-templates`); - return Api.get(TemplatesAPI.baseUrl + `/app-templates/${templateId}/similar`); + return Api.get(`${TemplatesAPI.baseUrl}/app-templates/${templateId}/similar`); - return Api.post(TemplatesAPI.baseUrl + `/app-templates/${templateId}/import/${workspaceId}`); + return Api.post(`${TemplatesAPI.baseUrl}/app-templates/${templateId}/import/${workspaceId}`); - return Api.post(TemplatesAPI.baseUrl + `/app-templates/${templateId}/merge/${applicationId}/${organizationId}`, body); + return Api.post(`${TemplatesAPI.baseUrl}/app-templates/${templateId}/merge/${applicationId}/${organizationId}`, body); - return Api.get(TemplatesAPI.baseUrl + `/app-templates/filters`); + return Api.get(`${TemplatesAPI.baseUrl}/app-templates/filters`); - return Api.post(TemplatesAPI.baseUrl + `/app-templates/publish/community-template`, body); + return Api.post(`${TemplatesAPI.baseUrl}/app-templates/publish/community-template`, body);Using template literals simplifies the syntax and improves readability.
Also applies to: 92-92, 99-99, 107-108, 118-119, 126-126, 132-132
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/src/api/TemplatesApi.ts (5 hunks)
Additional context used
Learnings (1)
app/client/src/api/TemplatesApi.ts (1)
User: saiprabhu-dandanayak PR: appsmithorg/appsmith#33911 File: app/client/src/api/TemplatesApi.ts:0-0 Timestamp: 2024-06-04T03:58:07.902Z Learning: User saiprabhu-dandanayak has confirmed the removal of the `any` type in the TypeScript code for better type safety in the `TemplatesApi.ts` file.
Biome
app/client/src/api/TemplatesApi.ts
[error] 74-136: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
[error] 81-81: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
[error] 81-81: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
[error] 92-92: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
[error] 99-99: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
[error] 107-108: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
[error] 118-119: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
[error] 126-126: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
[error] 126-126: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
[error] 132-132: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
[error] 132-132: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
| ): Promise<AxiosPromise<TemplatesResponse>> { | ||
| const state = store.getState(); | ||
| const templates = getTemplatesSelector(state); | ||
| const template = templates.find((template) => template.id === templateId); | ||
| if (template) { | ||
| return createTemplateResponse(template) as AxiosPromise<TemplatesResponse>; | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure proper error handling in getTemplateInformation.
The getTemplateInformation function currently lacks proper error handling mechanisms. Specifically, it does not handle the scenario where the template is not found in the Redux store or if the API call fails.
- Add error handling for the case when the template is not found in the Redux store.
- Add error handling for potential failures in the API call.
Analysis chain
Ensure proper error handling in getTemplateInformation.
While the function now fetches templates from the Redux store, it's crucial to ensure that error scenarios are handled appropriately, especially when the template is not found in the store.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify proper error handling when fetching templates.
# Test: Search for error handling patterns in the TemplatesApi.ts file. Expect: Proper error handling mechanisms.
rg --type ts 'catch|throw|error' app/client/src/api/TemplatesApi.ts
Length of output: 92
Script:
#!/bin/bash
# Description: Locate and inspect the getTemplateInformation function in TemplatesApi.ts for error handling.
# Locate the getTemplateInformation function
ast-grep --lang typescript --pattern 'function getTemplateInformation($_, $_) { $$$ }' app/client/src/api/TemplatesApi.ts
Length of output: 123
Script:
#!/bin/bash
# Description: Search for the getTemplateInformation function by name in TemplatesApi.ts.
# Search for the getTemplateInformation function
rg 'getTemplateInformation' app/client/src/api/TemplatesApi.ts
Length of output: 101
Script:
#!/bin/bash
# Description: Extract the full implementation of the getTemplateInformation function from TemplatesApi.ts.
# Extract the full implementation of the getTemplateInformation function
ast-grep --lang typescript --pattern 'static async getTemplateInformation($_, $_) { $$$ }' app/client/src/api/TemplatesApi.ts
Length of output: 601
Script:
#!/bin/bash
# Description: Extract the full implementation of the getTemplateInformation function from TemplatesApi.ts.
# Corrected pattern to extract the full implementation of the getTemplateInformation function
ast-grep --lang typescript --pattern 'class $_ { static async getTemplateInformation($_, $_) { $$$ } }' app/client/src/api/TemplatesApi.ts
Length of output: 140
Script:
#!/bin/bash
# Description: Search for the getTemplateInformation function definition and surrounding lines in TemplatesApi.ts.
# Search for the getTemplateInformation function definition and surrounding lines
rg -A 20 'static async getTemplateInformation' app/client/src/api/TemplatesApi.ts
Length of output: 815
jacquesikot
left a comment
jacquesikot
left a comment
There was a problem hiding this comment.
Hi @saiprabhu-dandanayak,
Thank you for contributing to our project. However, I have added some comments on areas where I have concerns. Please go through them and let me know if you need any assistance.
| responseTimeout: 60000, | ||
| pageLoadTimeout: 60000, | ||
| video: true, | ||
| videosFolder: "cypress/videos", |
There was a problem hiding this comment.
The video setting is already enabled, which means that Cypress will record videos of our tests. The videosFolder setting is also not necessary, as the default location for video recordings is cypress/videos.
Regarding the videoCompression setting, it's generally recommended to keep the default value of false unless you have specific requirements for video quality. Setting it to 32 can impact the performance of our tests, as the video compression process can slow down the test runtime.
There was a problem hiding this comment.
I would like to suggest a different approach to solving this problem.
Instead of directly accessing the API and checking the local state, I would recommend identifying the root cause of the redundant API call. This could involve tracing the code path that leads to the API call and understanding why it's being made in the first place and preventing it.
By addressing the root cause, you can ensure that the fix is more robust and less prone to unintended consequences. Additionally, this approach will help us better understand the code and make more informed decisions about how to improve it.
In this specific case, making changes to the TemplateApi could have unintended effects on other parts of the application. It's better to focus on the specific code path that's causing the redundant API call and fix it at that level.
If you're willing, I'd be happy to help you investigate the root cause of the API call and find a more targeted solution.
Thanks
There was a problem hiding this comment.
Hi @jacquesikot , Thanks for your suugestion , i will try to find the root cause for this additional API call.
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This PR has been closed because of inactivity. |
3 participants
Description of this PR
I have raised this PR
in order to remove redundant api call , and fetch the the templates from the redux storeSummary by CodeRabbit
New Features
Improvements