fix: Fix for regression in js query refactor in modules #34186
Conversation
WalkthroughThe changes across various files improve the handling and updating of JavaScript collections in a web application. Major modifications include refactoring JS collections, simplifying dummy JS functions, updating network status assertions in Cypress tests, and addressing a bug with updating JS function names in the module editor. These updates enhance the app’s functionality, maintainability, and user experience. Changes
Assessment against linked issues
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 (
|
github-actions
bot
added
Bug
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9467656439. |
|
Deploy-Preview-URL: https://ce-34186.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9468689943. |
dvj1988
added
ok-to-test
|
Deploy-Preview-URL: https://ce-34186.dp.appsmith.com |
![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 (2)
app/client/src/sagas/JSPaneSagas.ts (2)
Line range hint
185-199: This generator function doesn't contain a yield statement.- function* someGeneratorFunction() { + function someFunction() {Consider converting this generator function to a regular function if
yieldis not needed, as indicated by the static analysis tool.
Line range hint
463-463: Avoid redundant double-negation in boolean contexts.- !!jsCollection.isMainJSCollection + jsCollection.isMainJSCollectionRemoving the double-negation will make the code cleaner and easier to understand.
Also applies to: 504-504
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/src/sagas/JSPaneSagas.ts (9 hunks)
Additional context used
Biome
app/client/src/sagas/JSPaneSagas.ts
[error] 185-199: This generator function doesn't contain yield. (lint/correctness/useYield)
[error] 326-326: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 333-333: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 463-463: Avoid redundant double-negation. (lint/complexity/noExtraBooleanCast)
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
[error] 504-504: Avoid redundant double-negation. (lint/complexity/noExtraBooleanCast)
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
Additional comments not posted (5)
app/client/src/sagas/JSPaneSagas.ts (5)
210-224: Refactoring action dispatch logic appears to be correctly implemented.Ensure that the
refactorJSCollectionActionis thoroughly tested, especially since it involves multiple properties and potential edge cases.
Line range hint
319-360: The sagaupdateJSCollectionhandles the update process of JavaScript collections robustly.It's good to see comprehensive error handling and logging within this saga. Ensure that the success and error flows are covered by unit tests to prevent regressions in future changes.
Line range hint
636-684: ThehandleRefactorJSActionNameSagafunction is well-structured and handles the refactoring of JS action names effectively.This function is crucial for maintaining the integrity of JS collections when their actions are renamed. It would be beneficial to ensure that this function is covered by integration tests, given its impact on the application's functionality.
831-834: Integration ofhandleRefactorJSActionNameSagainto the root saga is correctly done.This ensures that the saga is correctly hooked into the application's saga middleware and will respond to the appropriate actions.
Line range hint
326-326: Consider using optional chaining to simplify the code.
[REFACTOR_SUGGESTion]- if (jsCollection && jsCollection.isMainJSCollection) + if (jsCollection?.isMainJSCollection)This change will make the code cleaner and prevent potential runtime errors due to undefined or null values.
Also applies to: 333-333
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
app/client/src/utils/JSPaneUtils.tsx (1)
Line range hint
150-150: Consider applying the recommendations from the static analysis tool:
- Remove redundant double-negation to simplify the boolean expressions.
- Use optional chaining to make the code more readable and safe against null and undefined values.
- if (!!someValue) { ... } + if (someValue) { ... } - someObject && someObject.someProperty ? someObject.someProperty : 'default' + someObject?.someProperty ?? 'default'Also applies to: 155-155
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/src/utils/JSPaneUtils.tsx (4 hunks)
Additional context used
Biome
app/client/src/utils/JSPaneUtils.tsx
[error] 150-150: Avoid redundant double-negation. (lint/complexity/noExtraBooleanCast)
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
[error] 155-155: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
Additional comments not posted (2)
app/client/src/utils/JSPaneUtils.tsx (2)
222-222: The simplification of dummy functions and values looks good. Please ensure that these changes do not impact any dependent functionalities.Also applies to: 234-234, 246-246, 250-250
274-274: The simplification of the function body increateSingleFunctionJsCollectionis consistent with the changes increateDummyJSCollectionActions. Please verify that this change does not affect any existing functionalities.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9481536388. |
dvj1988
added
ok-to-test
|
Deploy-Preview-URL: https://ce-34186.dp.appsmith.com |
There was a problem hiding this comment.
JSPaneUtils changes look good to me.
I would let the revert changes be reviewed by @ApekshaBhosale / @ashit-rath
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
app/client/cypress/support/Pages/JSEditor.ts (1)
Line range hint
246-267: Refactor the function expression to an arrow function for better readability and consistency.- private HandleJsContentFilling(toPaste: boolean, JSCode: string, el: any) { + private HandleJsContentFilling = (toPaste: boolean, JSCode: string, el: any) => { if (toPaste) { this.agHelper.Paste(el, JSCode); } else { cy.get(el).type(JSCode, { parseSpecialCharSequences: false, delay: 40, force: true, }); } }This change aligns with modern JavaScript practices and enhances the readability of the code by using arrow functions where the
thiscontext is not modified.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/cypress/support/Pages/JSEditor.ts (1 hunks)
Additional context used
Biome
app/client/cypress/support/Pages/JSEditor.ts
[error] 246-267: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
Additional comments not posted (1)
app/client/cypress/support/Pages/JSEditor.ts (1)
115-115: Update the network status assertion to match the new requirement.This change aligns with the PR's objective to update the network status assertion method to check for
@createNewJSCollectioninstead of@jsCollectionswhen creating a new JS collection. This is crucial for ensuring that the system correctly interprets the creation of new JavaScript collections.
dvj1988
added
ok-to-test
|
/ci-test-limit |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/9493885506. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
app/client/cypress/support/Pages/JSEditor.ts (1)
Line range hint
246-267: Consider refactoring this function expression to an arrow function for improved readability and reduced complexity.- private _onPageLoadSwitch = function (functionName: string) { + private _onPageLoadSwitch = (functionName: string) => { `.${functionName}-on-page-load-setting input[role="switch"]`; }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/client/cypress/limited-tests.txt (1 hunks)
- app/client/cypress/support/Pages/JSEditor.ts (1 hunks)
Additional context used
LanguageTool
app/client/cypress/limited-tests.txt
[uncategorized] ~1-~1: You might be missing the article “the” here. (AI_EN_LECTOR_MISSING_DETERMINER_THE)
Context: ... limited tests - give the spec names in below format: cypress/e2e/Regression/ClientSi...
[uncategorized] ~7-~7: You might be missing the article “the” here. (AI_EN_LECTOR_MISSING_DETERMINER_THE)
Context: ...ile to run minimum of specs. Do not run entire suite with this command.
Biome
app/client/cypress/support/Pages/JSEditor.ts
[error] 246-267: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
Additional comments not posted (2)
app/client/cypress/limited-tests.txt (1)
2-2: Change in test specification approved.app/client/cypress/support/Pages/JSEditor.ts (1)
115-115: Updated network status check aligns with the new API endpoint for creating JS collections.
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9493885506. |
dvj1988
added
ok-to-test
This reverts commit 3b7c67d.
PR #33545 removed the action `REFACTOR_JS_ACTION_NAME` and called the saga `handleRefactorJSActionNameSaga` directly. This caused a regression in the refactor of functions in js modules. This PR reverts this code. Fixes #34148 /ok-to-test tags="@tag.All" <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9494201779> > Commit: 3b7c67d > Cypress dashboard url: <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9494201779&attempt=1" target="_blank">Click here!</a> <!-- end of auto-generated comment: Cypress test results --> Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> - **New Features** - Added new actions for refactoring JavaScript collections to improve management and updating of JS collections in the application. - **Refactor** - Simplified dummy JavaScript functions, removing comments and unnecessary code to streamline function bodies. - Updated variable initialization values for cleaner code. - **Tests** - Updated network status assertions for JS collections in test scripts. - Switched test specifications in the Cypress test suite to ensure better coverage. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
PR #33545 removed the action `REFACTOR_JS_ACTION_NAME` and called the saga `handleRefactorJSActionNameSaga` directly. This caused a regression in the refactor of functions in js modules. This PR reverts this code. Fixes #34148 /ok-to-test tags="@tag.All" <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9494201779> > Commit: 3b7c67d > Cypress dashboard url: <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9494201779&attempt=1" target="_blank">Click here!</a> <!-- end of auto-generated comment: Cypress test results --> Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> - **New Features** - Added new actions for refactoring JavaScript collections to improve management and updating of JS collections in the application. - **Refactor** - Simplified dummy JavaScript functions, removing comments and unnecessary code to streamline function bodies. - Updated variable initialization values for cleaner code. - **Tests** - Updated network status assertions for JS collections in test scripts. - Switched test specifications in the Cypress test suite to ensure better coverage. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
PR #33545 removed the action
REFACTOR_JS_ACTION_NAMEand called the sagahandleRefactorJSActionNameSagadirectly. This caused a regression in the refactor of functions in js modules. This PR reverts this code.Fixes #34148
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/9494201779
Commit: 3b7c67d
Cypress dashboard url: Click here!
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Refactor
Tests