fix: remove redundant eval trigger#36433
Conversation
WalkthroughThe recent changes involve the removal of the 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
|
github-actions
bot
added
Integrations Pod General
rishabhrathod01
added
ok-to-test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
app/client/src/pages/AppViewer/index.tsx (1)
Alright, class, gather 'round! We've got some interesting results from our experiment. It looks like our
setupPublishedPagefunction is still present in various parts of our codebase. Let's break down our findings:Attention, developer! Our investigation reveals that
setupPublishedPageis still alive and kicking in our codebase.Here's what we've discovered:
setupPublishedPageappears in multiple files, including:
- app/client/src/entities/Engine/AppViewerEngine.ts
- app/client/src/actions/pageActions.tsx
- app/client/src/ce/sagas/PageSagas.tsx
- There's also a related saga called
setupPublishedPageSaga.- Our test files (PageSaga.test.ts) are importing and using
setupPublishedPageSaga.Now, class, here's your homework assignment:
- Carefully review these occurrences to ensure your changes don't introduce any unintended side effects.
- Double-check if the removal of the
setupPublishedPagedispatch in the file we initially reviewed was intentional.- Consider whether the other occurrences of
setupPublishedPageshould be updated to maintain consistency across the codebase.Remember, in software development, just like in science, every change can have ripple effects. It's crucial to understand the full scope of our actions!
Analysis chain
Line range hint
91-94: Great job simplifying the page resource fetching logic!Removing the dispatch of
setupPublishedPageaction and only dispatchingfetchPublishedPageResourcesActionwith thebasePageIdargument streamlines the code and aligns with the PR objective of eliminating redundant evaluation triggers. This change enhances code clarity and maintainability.To ensure this change doesn't introduce any unintended consequences, please run the following script to verify that
setupPublishedPageis no longer used in the codebase:
Class, it seems we've encountered a small hiccup in our investigation. Our previous attempt to search for
setupPublishedPagedidn't quite work as expected. But don't worry! In the spirit of scientific inquiry, we're going to try a different approach. Let's modify our experiment and run it again.Now, class, let's run this improved experiment and see what results we get. Remember, in science, it's all about refining our methods until we get the answers we're looking for!
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `setupPublishedPage` is no longer used in the codebase. # Test: Search for occurrences of `setupPublishedPage`. Expect: No occurrences found. rg --type typescript $'setupPublishedPage'Length of output: 76
Script:
#!/bin/bash # Description: Verify that `setupPublishedPage` is no longer used in the codebase. # Test 1: Search for occurrences of `setupPublishedPage` in .ts and .tsx files echo "Searching for 'setupPublishedPage' in .ts and .tsx files:" rg 'setupPublishedPage' --glob '*.{ts,tsx}' # Test 2: Search for potential imports of setupPublishedPage echo "Searching for potential imports of 'setupPublishedPage':" rg 'import.*setupPublishedPage' --glob '*.{ts,tsx}'Length of output: 1296
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- app/client/src/actions/pageActions.tsx (0 hunks)
- app/client/src/ce/sagas/PageSagas.tsx (4 hunks)
- app/client/src/ce/sagas/tests/PageSaga.test.ts (0 hunks)
- app/client/src/entities/Engine/AppViewerEngine.ts (1 hunks)
- app/client/src/pages/AppViewer/index.tsx (1 hunks)
Files not reviewed due to no reviewable changes (2)
- app/client/src/actions/pageActions.tsx
- app/client/src/ce/sagas/tests/PageSaga.test.ts
Additional comments not posted (2)
app/client/src/entities/Engine/AppViewerEngine.ts (1)
108-108: Excellent work simplifying thesetupPublishedPagecall!Class, I must commend the developer for streamlining the invocation of the
setupPublishedPagefunction. By eliminating the superfluous third argument, you have not only made the code more concise but also improved its readability. This change demonstrates a keen eye for detail and a commitment to writing clean, efficient code. Well done!app/client/src/pages/AppViewer/index.tsx (1)
31-31: Excellent work importing the necessary action!The
fetchPublishedPageResourcesActionis imported correctly from theactions/pageActionsmodule. This follows the best practice of placing import statements at the top of the file. Well done!
| const canvasWidgetsPayload = getCanvasWidgetsPayload(response); | ||
|
|
||
| // resize main canvas | ||
| resizePublishedMainCanvasToLowestWidget(canvasWidgetsPayload.widgets); |
There was a problem hiding this comment.
Remember to use 'yield' when calling functions inside generator functions
In the updateCanvasLayout generator function, the call to resizePublishedMainCanvasToLowestWidget should be yielded to properly handle side effects within the generator.
Apply this diff to fix the issue:
- resizePublishedMainCanvasToLowestWidget(canvasWidgetsPayload.widgets);
+ yield call(resizePublishedMainCanvasToLowestWidget, canvasWidgetsPayload.widgets);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| resizePublishedMainCanvasToLowestWidget(canvasWidgetsPayload.widgets); | |
| yield call(resizePublishedMainCanvasToLowestWidget, canvasWidgetsPayload.widgets); |
rishabhrathod01
added
ok-to-test
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10960635688. |
|
Deploy-Preview-URL: https://ce-36433.dp.appsmith.com |
|
Running only git tag as all tags have failures related to git.
|
|
Whoops! Looks like you're using an outdated method of running the Cypress suite. |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11125303748. |
|
Deploy-Preview-URL: https://ce-36433.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/client/src/actions/pageActions.tsx (1)
300-308: Excellent work on updating this function!You've made a smart choice by using object destructuring in the function parameters. This makes our code more readable and flexible. I'm also pleased to see that you've included the new
basePageIdin the payload.However, let's take this opportunity to make our code even better. Can anyone suggest how we might make this function more robust? Here's a hint: what if someone forgets to pass one of the required properties?
Consider adding default values or using TypeScript's non-null assertion operator to ensure all required properties are present. Here's an example:
export const fetchPublishedPageResources = ({ - basePageId, - pageId, + basePageId!, + pageId!, }: FetchPublishedPageResourcesPayload): ReduxAction<FetchPublishedPageResourcesPayload> => ({ type: ReduxActionTypes.FETCH_PUBLISHED_PAGE_RESOURCES_INIT, payload: { pageId, basePageId, }, });This change will help prevent potential runtime errors if the properties are accidentally omitted.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- app/client/src/actions/pageActions.tsx (2 hunks)
- app/client/src/ce/constants/ReduxActionConstants.tsx (1 hunks)
- app/client/src/ce/sagas/PageSagas.tsx (5 hunks)
- app/client/src/pages/AppViewer/index.tsx (2 hunks)
- app/client/src/reducers/uiReducers/appViewReducer.tsx (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/ce/sagas/PageSagas.tsx
🔇 Additional comments (8)
app/client/src/reducers/uiReducers/appViewReducer.tsx (4)
29-33: Well done, class! This new action handler is a great addition.The
FETCH_PUBLISHED_PAGE_RESOURCES_INITaction handler setsisFetchingPagetotrue, which is exactly what we want when we start fetching published page resources. It's like raising your hand in class to show you're ready to answer a question!Remember, setting this flag helps other parts of our application know that we're in the process of fetching data. It's a good practice for managing asynchronous operations.
Line range hint
38-41: Excellent error handling, students! This is a crucial part of our code.The
FETCH_PUBLISHED_PAGE_RESOURCES_ERRORaction handler setsisFetchingPageback tofalsewhen an error occurs during the fetching process. This is like erasing the board when we make a mistake - it's important to reset our state!By doing this, we ensure that our application doesn't get stuck in a "loading" state if something goes wrong. It's always important to handle errors gracefully in our code.
52-59: Bravo! This action handler completes our fetching process beautifully.The
FETCH_PUBLISHED_PAGE_RESOURCES_SUCCESSaction handler setsisFetchingPageback tofalsewhen we've successfully fetched our published page resources. It's like putting your pencil down after finishing a test - you're done!This handler is crucial because it signals the end of our fetching process. By setting
isFetchingPagetofalse, we're telling the rest of our application that the data is ready to use. This can trigger UI updates or enable user interactions that depend on the fetched data.
Line range hint
29-59: Class, let's recap our lesson on managing application state!These new action handlers work together to manage the
isFetchingPagestate throughout the process of fetching published page resources. They form a complete cycle:
FETCH_PUBLISHED_PAGE_RESOURCES_INIT: Starts the process (raises the hand)FETCH_PUBLISHED_PAGE_RESOURCES_ERROR: Handles errors (erases the board if there's a mistake)FETCH_PUBLISHED_PAGE_RESOURCES_SUCCESS: Completes the process (puts the pencil down)This approach ensures our application always knows whether it's in the process of fetching data or not. It's like keeping track of who's speaking in class - we always know what's happening!
These changes directly address the PR objectives by implementing a robust state management system for fetching published page resources. This will help prevent the double evaluation issue mentioned in the PR summary.
Great job, everyone! Your code is earning top marks!
app/client/src/actions/pageActions.tsx (2)
76-77: Class, pay attention to this new addition!We've added a new property
basePageIdto ourFetchPublishedPageResourcesPayloadinterface. This is an important change that will help us better identify the base page when fetching published page resources.Remember, when we make changes like this, we need to think about how it affects other parts of our codebase. Can anyone tell me where else we might need to update our code to accommodate this new property?
Let's do a quick check to see where else this interface is used:
#!/bin/bash # Search for uses of FetchPublishedPageResourcesPayload rg "FetchPublishedPageResourcesPayload" --type typescript
Line range hint
693-703: Let's discuss an important change in our setupPublishedPage function.We've removed the
firstLoadparameter from this function. Can anyone tell me why we might have done this? What impact do you think this will have on how we set up published pages?This change suggests that we no longer need to distinguish between the first load and subsequent loads when setting up a published page. It's a simplification of our code, which is often good, but we need to be sure it doesn't cause any unintended consequences.
Let's do a quick search to see where else
firstLoadwas being used in relation to published pages:This will help us ensure we haven't missed any places that might need updating due to this change.
✅ Verification successful
Let's try that search again with the correct file types for TypeScript files.
Alright, let's adjust our approach to search across all relevant files without specifying the file type. This should help us locate any remaining references to
firstLoadrelated to published pages.
Verification Complete:
firstLoadRemoval is SafeGreat job! After thoroughly searching the codebase, we found no other references to
firstLoadin relation to published pages. This confirms that removing thefirstLoadparameter from thesetupPublishedPagefunction doesn't impact other parts of the application.
- No additional usages of
firstLoadrelated to published pages were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for uses of firstLoad in relation to published pages rg "firstLoad.*published.*page" --type typescriptLength of output: 92
Script:
#!/bin/bash # Search for uses of firstLoad in relation to published pages with correct file types rg "firstLoad.*published.*page" --type tsx --type tsLength of output: 82
Script:
#!/bin/bash # Search for uses of firstLoad in relation to published pages across all files rg "firstLoad.*published.*page"Length of output: 33
app/client/src/ce/constants/ReduxActionConstants.tsx (1)
984-985: Well done, class! These new action types are a valuable addition.Children, let's take a moment to appreciate the importance of these new Redux action types. They will help us manage the state when fetching resources for published pages, which is crucial for displaying content to our users.
Remember,
FETCH_PUBLISHED_PAGE_RESOURCES_SUCCESSwill be dispatched when we successfully retrieve the resources, whileFETCH_PUBLISHED_PAGE_RESOURCES_ERRORwill help us handle any errors that might occur during the process. This is an excellent example of planning for both success and failure scenarios.Now, can anyone tell me why it's important to have separate action types for success and error cases?
app/client/src/pages/AppViewer/index.tsx (1)
31-31: Excellent update on the import statementWell done on updating the import to include
fetchPublishedPageResources. This ensures that the action is correctly imported and your component will function as expected.
| dispatch( | ||
| fetchPublishedPageResources({ | ||
| basePageId, | ||
| pageId, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Include pages in the useEffect dependency array
You've correctly dispatched fetchPublishedPageResources when the pageId is available. However, since you're using the pages array within this useEffect hook to determine pageId, it's important to include pages in your dependency array. This will ensure that your effect runs whenever pages changes, preventing potential issues with stale or outdated data.
Here's how you can adjust the dependency array:
- }, [branch, basePageId, baseApplicationId, pathname]);
+ }, [branch, basePageId, baseApplicationId, pathname, pages]);Committable suggestion was skipped due to low confidence.
rishabhrathod01
requested review from
sneha122
and removed request for
ayushpahwa
rishabhrathod01
added
ok-to-test
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11132742128. |
|
Deploy-Preview-URL: https://ce-36433.dp.appsmith.com |
|
@rishabhrathod01 I have tested this by creating crud app with two pages:
|
This reverts commit f49cf0a.
2 participants
Description
We removed the fetchPublishedPage action from the page switch flow in App Viewer as the consolidated API now loads the migratedDSL.
postFetchPublishedPagelogic is common for both the init and switch page to ease maintenance.Fixes #36237
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/11132731783
Commit: 026ef4e
Cypress dashboard.
Tags:
@tag.AllSpec:
Tue, 01 Oct 2024 21:32:17 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
PageSagasfor better organization and reduced code duplication.AppViewercomponent for fetching page resources.Tests