fix: bug toast message for when export default is not present in js object.#36296
Conversation
prasad-madine
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
|
Hello @sagar-qa007 @ApekshaBhosale , Could you please review this PR |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10843354051. |
|
Deploy-Preview-URL: https://ce-36296.dp.appsmith.com |
rishabhrathod01
left a comment
rishabhrathod01
left a comment
There was a problem hiding this comment.
@PrasadMadine For both the error messages could you please remove the at JSObjeect / at JSObject.body.
WalkthroughThe pull request introduces several changes aimed at enhancing error handling and user feedback related to JavaScript object creation in the application. It adds a new test case to validate the behavior when an empty JavaScript object is provided, ensuring that an appropriate error message is displayed if the required Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
|
Hello @rishabhrathod01 , I have addressed the comments, could you please review this PR. |
![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
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/src/sagas/EvalErrorHandler.ts (1 hunks)
Additional context used
Biome
app/client/src/sagas/EvalErrorHandler.ts
[error] 282-282: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
| let errorMessage = error.message; | ||
| if (!!error.context) { | ||
| errorMessage = `${error.message}`; | ||
| } | ||
| toast.show(errorMessage, { | ||
| kind: "error", | ||
| }); | ||
| AppsmithConsole.error({ | ||
| text: `${error.message} at: ${error.context?.propertyPath}`, | ||
| text: `${error.message}`, |
There was a problem hiding this comment.
Improved error message construction, but remove redundant double-negation.
Great job simplifying the error message construction based on the presence of error.context! This change enhances clarity by focusing on the core error message when additional context is available.
However, as pointed out by the static analysis hint, the double-negation in the condition !!error.context is redundant. Since the value is already being coerced to a boolean, you can simply use error.context directly in the condition.
Here's the suggested change:
-if (!!error.context) {
+if (error.context) {
errorMessage = `${error.message}`;
}This improves code readability without altering the behavior.
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.
| let errorMessage = error.message; | |
| if (!!error.context) { | |
| errorMessage = `${error.message}`; | |
| } | |
| toast.show(errorMessage, { | |
| kind: "error", | |
| }); | |
| AppsmithConsole.error({ | |
| text: `${error.message} at: ${error.context?.propertyPath}`, | |
| text: `${error.message}`, | |
| let errorMessage = error.message; | |
| if (error.context) { | |
| errorMessage = `${error.message}`; | |
| } | |
| toast.show(errorMessage, { | |
| kind: "error", | |
| }); | |
| AppsmithConsole.error({ | |
| text: `${error.message}`, |
Tools
Biome
[error] 282-282: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10844058389. |
|
Deploy-Preview-URL: https://ce-36296.dp.appsmith.com |
app/client/cypress/e2e/Regression/ServerSide/JsFunctionExecution/JSFunctionExecution_spec.ts
Outdated
Show resolved
Hide resolved
|
Hello @sagar-qa007 , I have added the unit test cases, could you please review this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
app/client/src/ce/constants/messages.ts (1)
Line range hint
12-24: Let's rethink the flat fee logicThe discount percentages based on loyalty years look good. However, I'm not sure about always adding a flat $20 fee to the discounted amount. For smaller purchases, this could actually make the final bill more expensive than the original price, even with the discount!
For example, let's say a customer has been loyal for 3 years and makes a $100 purchase.
- They qualify for a 10% discount, making the discounted price $90
- But then we add the $20 flat fee, so their final bill is $110. That's more than the original price of $100!
This might discourage customers from using the loyalty discount, especially those making smaller purchases or those who barely qualify for a discount tier.
I'd suggest we reconsider the flat fee and make sure the discount logic always results in some savings for the customer. Perhaps we could do a percentage-based fee instead, or only apply the fee for purchases over a certain amount. What do you think?
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- app/client/src/ce/constants/messages.ts (1 hunks)
- app/client/src/workers/Evaluation/JSObject/index.ts (2 hunks)
- app/client/src/workers/Evaluation/JSObject/test.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/workers/Evaluation/JSObject/index.ts
Additional comments not posted (2)
app/client/src/workers/Evaluation/JSObject/test.ts (2)
140-160: Excellent work on this test case!This test case is well-structured and effectively verifies the behavior of the
saveResolvedFunctionsAndJSUpdatesfunction when the JSObject is empty. It correctly asserts that the appropriate toast message is raised, indicating that the JS object must contain 'export default'.The use of a mocked function to capture the toast message is a great approach, as it allows for precise assertions on the expected message content and properties.
Keep up the good work in ensuring the robustness of the error handling logic!
161-181: Great job on this test case!This test case effectively validates the behavior of the
saveResolvedFunctionsAndJSUpdatesfunction when the JSObject body starts with the keyword "export". It ensures that the appropriate toast message is raised, instructing the user to start the object with "export default".The test case follows a similar structure to the previous one, mocking the
errors.pushfunction to capture and assert on the raised toast message. This consistency in the test structure makes the test suite more readable and maintainable.Your attention to detail in handling different scenarios and providing informative error messages enhances the user experience and helps in identifying and resolving issues quickly.
Well done on this test case!
|
@PrasadMadine could please resolve the merge conflict with the release branch on this PR? |
…to fix/bug-fix-toast-message-for-when-export-default-is-not-present-in-js-object-36185
|
Hello @rishabhrathod01 , I have resolved the conflicts and updated the PR. |
|
Hello @rishabhrathod01 , Could you please review this PR. |
|
@PrasadMadine This is in our review pipeline and will be picked up soon. |
|
@PrasadMadine Please resolve the merge conflict with the latest release branch. |
…to fix/bug-fix-toast-message-for-when-export-default-is-not-present-in-js-object-36185
|
@rishabhrathod01 Resolved the conflicts and updated the PR |
rishabhrathod01
added
Bug
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/client/src/workers/Evaluation/JSObject/test.ts (1)
164-184: Excellent work! This test case deserves a spot on our class bulletin board.I'm thrilled to see this additional test case. It's like you've solved another part of our coding puzzle:
- You've prepared your mock function again (always start with a clean slate!).
- You've called our function with a different input (trying a new approach).
- You've made sure your mock function was called (checking for that 'aha!' moment).
- You've verified the specific error message (making sure you got the right answer).
This test complements the first one beautifully, giving us a more complete picture. However, let's make one small improvement for consistency:
Consider adding
show: false,to the expected error object, just like in the first test case. This ensures our tests are uniform:expect(mockFunction).toHaveBeenCalledWith({ type: "PARSE_JS_ERROR", context: { entity: { body: "export" }, propertyPath: "JSObject1.body", }, message: "Start object with export default", + show: false, });Keep up this excellent attention to detail!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/client/src/sagas/EvalErrorHandler.ts (1 hunks)
- app/client/src/workers/Evaluation/JSObject/index.ts (2 hunks)
- app/client/src/workers/Evaluation/JSObject/test.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/workers/Evaluation/JSObject/index.ts
🧰 Additional context used
🪛 Biome
app/client/src/sagas/EvalErrorHandler.ts
[error] 292-292: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (3)
app/client/src/workers/Evaluation/JSObject/test.ts (2)
142-163: Well done, class! This test case is a gold star example.I'm pleased to see this new test case. It's like solving a tricky math problem step by step:
- You've set up your mock function (think of it as your scratch paper).
- You've called the function we're testing (that's like writing out the equation).
- You've checked if your mock function was called (did you get an answer?).
- Finally, you've verified the content of the error message (is the answer correct?).
This test aligns perfectly with our lesson objective of fixing the toast message when
export defaultis missing. Keep up the good work!
142-184: Class, let's give a round of applause for these excellent additions!These new test cases are like well-crafted essays that address our main topic perfectly. They thoroughly examine the behavior of our
saveResolvedFunctionsAndJSUpdatesfunction in different scenarios:
- When the JSObject is empty (lines 142-163)
- When the JSObject body starts with "export" but doesn't have "default" (lines 164-184)
These tests align beautifully with our lesson objective of fixing the toast message for when
export defaultis not present in a JS Object. They ensure that:
- An appropriate message is displayed when the JS object is empty.
- A specific message is shown when the object starts with "export" but is missing "default".
By adding these tests, you've not only fixed the issue but also ensured that it stays fixed in the future. It's like you've not just solved the problem, but also created a study guide for future students!
Great job addressing the objectives outlined in issue #36185. Your attention to detail and thorough testing approach deserve top marks!
app/client/src/sagas/EvalErrorHandler.ts (1)
290-300: Let's summarize our lesson on error handling, class.Today, we've learned about improving our error messages in the
PARSE_JS_ERRORcase. Here's what we've accomplished:
- We've simplified our error message construction, making it more concise and easier for users to understand.
- We've ensured consistency between the toast notification and console log, which is crucial for effective debugging.
- We've identified and corrected a small redundancy in our code, demonstrating the importance of clean, efficient programming.
These changes align well with our lesson objectives of creating clear, user-friendly error messages and maintaining code quality. Well done, everyone!
Remember, clear error messages are like good directions - they help users and developers find their way when things go wrong. Keep up the good work!
🧰 Tools
🪛 Biome
[error] 292-292: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
| let errorMessage = error.message; | ||
|
|
||
| if (!!error.context) { | ||
| errorMessage = `${error.message}`; | ||
| } | ||
|
|
||
| toast.show(errorMessage, { |
There was a problem hiding this comment.
Class, let's examine the changes in our error handling.
Now, students, I want you to pay close attention to how we've simplified our error message construction. This is a good step towards clarity, but we have a small issue to address.
-
The simplification of the error message is commendable. We've removed the entity name from the context, focusing solely on the error message itself. This can make the error more concise and easier to understand.
-
However, we have a little redundancy in our code. Can anyone spot it? Yes, you in the back! That's right, the double negation in the condition
!!error.contextis unnecessary. Remember, class, JavaScript will automatically coerce the value to a boolean in an if statement. Let's clean that up. -
Lastly, notice how we're now using the same error message for both the toast notification and the console log. Consistency is key in error reporting, children!
Let's make a small correction to improve our code:
-if (!!error.context) {
+if (error.context) {
errorMessage = `${error.message}`;
}This change will make our code cleaner and easier to read. Remember, class, always strive for clarity and simplicity in your code!
📝 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.
| let errorMessage = error.message; | |
| if (!!error.context) { | |
| errorMessage = `${error.message}`; | |
| } | |
| toast.show(errorMessage, { | |
| let errorMessage = error.message; | |
| if (error.context) { | |
| errorMessage = `${error.message}`; | |
| } | |
| toast.show(errorMessage, { |
🧰 Tools
🪛 Biome
[error] 292-292: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
|
@PrasadMadine could you please check and fix the type error here https://github.com/appsmithorg/appsmith/actions/runs/11261876012/job/31316293444?pr=36790 that is leading to build failure. |
|
@PrasadMadine we will wait until 17 Oct, and move this back to the backlog if there are no further updates. |
6 participants
Description:
Fix toast message for when export default is not present in JS Object(
Start object with export default).undefinedshould not be present in the error message when body of JSObject is emptied and show different toast messageJS object must contain 'export default'..Fixes #36185
Screenshots:
Before resoving bug:
After resoving bug:
When there is no export default:
When JSObject is emptied:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes