fix: JSObject diff error when actions are missing #38572
Conversation
WalkthroughThe pull request introduces modifications to two files: Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Finishing Touches
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
|
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12801916786. |
|
Deploy-Preview-URL: https://ce-38572.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12802470736. |
![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
🧹 Nitpick comments (1)
app/client/src/utils/JSPaneUtils.tsx (1)
54-56: LGTM! Defensive null check added.The null check prevents the TypeError when actions are undefined. Consider using optional chaining for a more modern approach.
- const preExisted = (jsAction.actions || []).find( + const preExisted = jsAction.actions?.find( (js) => js.name === action.name, );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/sagas/JSPaneSagas.ts(1 hunks)app/client/src/utils/JSPaneUtils.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (1)
app/client/src/sagas/JSPaneSagas.ts (1)
211-216: LGTM! Proper initialization of actions array.This change ensures that the actions array is always initialized before any operations are performed on it, preventing potential null reference errors.
|
Deploy-Preview-URL: https://ce-38572.dp.appsmith.com |
## Description This PR fixes 2 issues <em>Issue 1<em> - No function available in the dropdown of a valid JS Module in JS Module editor Due to some reason the actions property in the JSObject is missing instead of being `[]`, due to this when the check is done for getting the diff `const preExisted = jsAction.actions.find((js) => js.name === action.name);` the `.find` is on an `undefined` value. The actual reason of the `actions` to be missing is yet to be discovered but this is added to add guard against such failures. <em>Issue 2<em> - When a function is removed from a JSModule, a silent error is thrown by the splice `TypeError: Cannot delete property '0' of [object Array]` The reason for this is that the jsObject are directly being mutated for an object where the `writable` property is `false`. Upon verifying the code block removed from line 145-155 has no use since it just updates the jsActions object which neither is used by subsequent code of the function nor the caller of this function. Also splicing directly on redux selector data mutates the redux store directly which is a bad practice. So technically it's a piece of code that is unused, thus removing it fixes the problem Fixes appsmithorg#38683 ## Automation /ok-to-test tags="@tag.All" ### 🔍 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/12802464049> > Commit: 316f0ae > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12802464049&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Thu, 16 Jan 2025 11:54:27 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 - **Bug Fixes** - Improved error handling in JS collection updates by ensuring proper array initialization - Enhanced action comparison logic to handle undefined action arrays more gracefully - **Refactor** - Simplified action comparison and deletion process in JS collection utilities <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
This PR fixes 2 issues
Issue 1 - No function available in the dropdown of a valid JS Module in JS Module editor
Due to some reason the actions property in the JSObject is missing instead of being
[], due to this when the check is done for getting the diffconst preExisted = jsAction.actions.find((js) => js.name === action.name);the.findis on anundefinedvalue. The actual reason of theactionsto be missing is yet to be discovered but this is added to add guard against such failures.Issue 2 - When a function is removed from a JSModule, a silent error is thrown by the splice
TypeError: Cannot delete property '0' of [object Array]The reason for this is that the jsObject are directly being mutated for an object where the
writableproperty isfalse.Upon verifying the code block removed from line 145-155 has no use since it just updates the jsActions object which neither is used by subsequent code of the function nor the caller of this function.
Also splicing directly on redux selector data mutates the redux store directly which is a bad practice.
So technically it's a piece of code that is unused, thus removing it fixes the problem
Fixes #38683
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/12802464049
Commit: 316f0ae
Cypress dashboard.
Tags:
@tag.AllSpec:
Thu, 16 Jan 2025 11:54:27 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Bug Fixes
Refactor