Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Bug]: Cyclic dependency error is not thrown for cyclic dependency of js object with widget #37129

Open
1 task done
sneha122 opened this issue Oct 30, 2024 · 3 comments
Open
1 task done

[Bug]: Cyclic dependency error is not thrown for cyclic dependency of js object with widget #37129

sneha122 opened this issue Oct 30, 2024 · 3 comments
Assignees
@Nikhil-Nandagopal @pranavkanade @rohan-arthur @carinanfonseca
Labels
Bug Something isn't working Inviting Contribution Issues that we would like contributions to Javascript Product Issues related to users writing javascript in appsmith JS Objects Issues related to JS Objects Medium Issues that frustrate users due to poor UX Needs Triaging Needs attention from maintainers to triage Production Query & JS Pod Issues related to the query & JS Pod

Comments

@sneha122
Copy link
Contributor

sneha122 commented Oct 30, 2024 •
edited
Loading

Is there an existing issue for this?

  • I have searched the existing issues

Description

When JS object is edited to create a cyclic dependency with a widget, no toast message is thrown, but when the js object is edited again to remove the cyclic dependency, then we get error.

Screen.Recording.2024-10-30.at.12.18.19.PM.mov

Ideally, we should get error only when we have cyclic dependency and not on the removal of it.

Steps To Reproduce

  1. Add a text widget on canvas
  2. Add text widget's textData as {{JSObject1.myFun1.data}}
  3. Go to JsObject1
  4. Edit myFun1 to return Text1.text

Public Sample App

No response

Environment

Production

Severity

Medium (Frustrating UX)

Issue video log

No response

Version

Self hosted - v1.45
@sneha122 sneha122 added Bug Something isn't working Needs Triaging Needs attention from maintainers to triage labels Oct 30, 2024
@Nikhil-Nandagopal Nikhil-Nandagopal added Medium Issues that frustrate users due to poor UX Production labels Oct 30, 2024
@sneha122 sneha122 added JS Objects Issues related to JS Objects Javascript Product Issues related to users writing javascript in appsmith Query & JS Pod Issues related to the query & JS Pod labels Oct 30, 2024
@sneha122 sneha122 added the Inviting Contribution Issues that we would like contributions to label Oct 30, 2024
@sneha122 sneha122 mentioned this issue Oct 30, 2024
Merged
2 tasks
@ALOK9442
Copy link
Contributor

hey @sneha122 , I am debugging the issue and from what I understand, the current behavior is that when there's a circular dependency, we aren't seeing an error immediately. Instead, the error toast message only appears when we comment it out. Ideally, we should trigger the toast error message as soon as the circular dependency is detected, not just after commenting it out.

Is that correct, or am I missing something?

@ALOK9442
Copy link
Contributor

hey @sneha122 , I would like to work on this issue, could you please assign it to me

@sneha122
Copy link
Contributor Author

sneha122 commented Nov 4, 2024

Thanks @ALOK9442 for your interest in this issue! Can you please describe the approach you have in mind in order to resolve this issue?

sneha122 added a commit that referenced this issue Nov 6, 2024
@sneha122
fix: skipped unnecessary code for js object updates (#37125)
756dc54 
## Description

This PR is in continuation to
[PR](#37062) which we had
merged earlier, In previous, we skipped the redundant calls to update
page layout when updating each js object action, as we already have a
call for [updating the page layout for
actionCollection](https://github.com/appsmithorg/appsmith/blob/27bdeb92b6a0b27e6afcbfe80a5cb0705c0812ac/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java#L411)

Since we have skipped the update page layout for each js action, we no
longer need the code part after this, which basically fetches page data
from DB and updates the `errorReports` in actionDTO based on layout
`layoutOnLoadActionErrors`. This PR skips this unnecessary part too for
each js action as we do [set the errorReport for actionCollection in the
end](https://github.com/appsmithorg/appsmith/blob/27bdeb92b6a0b27e6afcbfe80a5cb0705c0812ac/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java#L430)

### Will this have any impact on error messages shown to user?
In order to understand this, checked out the frontend code to see if
errorReports from individual js action is getting consumed on updating
js object, looks like [it is
not](https://github.com/appsmithorg/appsmith/blob/e7e3d5e00290919c1df0767fdefed67458ec3cc9/app/client/src/sagas/JSPaneSagas.ts#L316),
so we can safely remove this piece of code.
However this points to existing bug in the code (as errorReports is not
even getting consumed from actionCollection), which is, when there is
cyclic dependency created for js object with a widget, we don't get any
toast message. Since this is existing issue which is not caused by any
of the above PR implementations, creating separate issue for it and
tracking it [here](#37129)

Fixes #37114 
_or_  
Fixes `Issue URL`
> [!WARNING]  
> _If no issue exists, please create an issue first, and check with the
maintainers if the issue is valid._

## Automation

/ok-to-test tags="@tag.JS, @tag.JS"

### 🔍 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/11698258739>
> Commit: 9fbde99
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11698258739&attempt=1"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.JS, @tag.JS`
> Spec:
> <hr>Wed, 06 Nov 2024 06:50:05 UTC
<!-- end of auto-generated comment: Cypress test results  -->


## Communication
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
-->
## Summary by CodeRabbit

- **New Features**
- Streamlined layout update process for actions, enhancing performance
and clarity.

- **Bug Fixes**
- Improved test reliability by monitoring interactions with the layout
service and handling cyclic dependencies.

- **Documentation**
- Updated comments to clarify the logic behind layout updates for JS
actions.

- **Tests**
- Enhanced test descriptions and assertions for better clarity and
validation of method interactions, including cyclic dependency
scenarios.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: “sneha122” <“[email protected]”>
github-actions bot pushed a commit to Zeral-Zhang/appsmith that referenced this issue Nov 20, 2024
@sneha122
fix: skipped unnecessary code for js object updates (appsmithorg#37125)
300a3bd 
## Description

This PR is in continuation to
[PR](appsmithorg#37062) which we had
merged earlier, In previous, we skipped the redundant calls to update
page layout when updating each js object action, as we already have a
call for [updating the page layout for
actionCollection](https://github.com/appsmithorg/appsmith/blob/27bdeb92b6a0b27e6afcbfe80a5cb0705c0812ac/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java#L411)

Since we have skipped the update page layout for each js action, we no
longer need the code part after this, which basically fetches page data
from DB and updates the `errorReports` in actionDTO based on layout
`layoutOnLoadActionErrors`. This PR skips this unnecessary part too for
each js action as we do [set the errorReport for actionCollection in the
end](https://github.com/appsmithorg/appsmith/blob/27bdeb92b6a0b27e6afcbfe80a5cb0705c0812ac/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java#L430)

### Will this have any impact on error messages shown to user?
In order to understand this, checked out the frontend code to see if
errorReports from individual js action is getting consumed on updating
js object, looks like [it is
not](https://github.com/appsmithorg/appsmith/blob/e7e3d5e00290919c1df0767fdefed67458ec3cc9/app/client/src/sagas/JSPaneSagas.ts#L316),
so we can safely remove this piece of code.
However this points to existing bug in the code (as errorReports is not
even getting consumed from actionCollection), which is, when there is
cyclic dependency created for js object with a widget, we don't get any
toast message. Since this is existing issue which is not caused by any
of the above PR implementations, creating separate issue for it and
tracking it [here](appsmithorg#37129)

Fixes appsmithorg#37114 
_or_  
Fixes `Issue URL`
> [!WARNING]  
> _If no issue exists, please create an issue first, and check with the
maintainers if the issue is valid._

## Automation

/ok-to-test tags="@tag.JS, @tag.JS"

### 🔍 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/11698258739>
> Commit: 9fbde99
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11698258739&attempt=1"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.JS, @tag.JS`
> Spec:
> <hr>Wed, 06 Nov 2024 06:50:05 UTC
<!-- end of auto-generated comment: Cypress test results  -->


## Communication
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
-->
## Summary by CodeRabbit

- **New Features**
- Streamlined layout update process for actions, enhancing performance
and clarity.

- **Bug Fixes**
- Improved test reliability by monitoring interactions with the layout
service and handling cyclic dependencies.

- **Documentation**
- Updated comments to clarify the logic behind layout updates for JS
actions.

- **Tests**
- Enhanced test descriptions and assertions for better clarity and
validation of method interactions, including cyclic dependency
scenarios.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: “sneha122” <“[email protected]”>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Nikhil-Nandagopal Nikhil-Nandagopal

@pranavkanade pranavkanade

@rohan-arthur rohan-arthur

@carinanfonseca carinanfonseca

Labels
Bug Something isn't working Inviting Contribution Issues that we would like contributions to Javascript Product Issues related to users writing javascript in appsmith JS Objects Issues related to JS Objects Medium Issues that frustrate users due to poor UX Needs Triaging Needs attention from maintainers to triage Production Query & JS Pod Issues related to the query & JS Pod
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@Nikhil-Nandagopal @pranavkanade @sneha122 @rohan-arthur @ALOK9442 @carinanfonseca