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.

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

[Bug]-[540]:storeValue() drops values when called in quick succession #7576

Open
zzgvh opened this issue Sep 17, 2021 · 10 comments
Open

[Bug]-[540]:storeValue() drops values when called in quick succession #7576

zzgvh opened this issue Sep 17, 2021 · 10 comments
Assignees
Labels
Bug Something isn't working Javascript Product Issues related to users writing javascript in appsmith JS Evaluation Issues related to JS evaluation on the platform Medium Issues that frustrate users due to poor UX Production Query & JS Pod Issues related to the query & JS Pod

Comments

@zzgvh
Copy link

zzgvh commented Sep 17, 2021

Description

I'm using appsmith.store to gather data that is ultimately used when posting a form. The store is also used to populate the form fields when certain events happen on the page.

However the use of the input fields that have onTextChanged events that call storeValue() suffer from a kind of slowness and ultimately dropped updates to the store it seems. This leads to values that you thought were entered being incorrect.

slow_input_bug.mp4

Note that I enter the number in the first line slowly and you can see how the store updates for each keystroke in the debug text field below the form. When I edit the second line to input 50 quickly the second storeValue() call seem to be dropped; you can see that the keystroke registers in the UI, but the update of the store seems not to happen leading to the input reverting the value 5 since that is the value of the store. This happens more easily in Firefox than in Chrome, but I can trigger it there too.

Demo app
(rename to .json, Github doesn't allow the uplaod of JSON for some reason...)

Important Details

  • Version: Cloud / Self-Hosted
  • OS: I'm mostly on Linux. Mint 19.
  • Browser: I see this in both Firefox and Chrome on Linux
  • Environment production
@zzgvh zzgvh added Bug Something isn't working Needs Triaging Needs attention from maintainers to triage labels Sep 17, 2021
@Nikhil-Nandagopal Nikhil-Nandagopal added High This issue blocks a user from building or impacts a lot of users JS Evaluation Issues related to JS evaluation on the platform and removed Needs Triaging Needs attention from maintainers to triage labels Sep 21, 2021
@Nikhil-Nandagopal
Copy link
Contributor

@zzgvh this is because the storeValue is async today. We'll be introducing a synchronous API which is non persistent

@hetunandu
Copy link
Member

@zzgvh This is happening because onTextChange is called in a debounced fashion. It has nothing to do with storeValue from what I can see. In my testing, I saw that updates were delayed but the order was correct.

@zzgvh
Copy link
Author

zzgvh commented Sep 30, 2021

Hey @hetunandu. I don't quite understand what you mean by onTextChange being called in a debounced fasion. Is there anything I can do to fix or mitigate the problem?

@Nikhil-Nandagopal Nikhil-Nandagopal added the Javascript Product Issues related to users writing javascript in appsmith label Oct 6, 2021
@hetunandu
Copy link
Member

Hey @hetunandu. I don't quite understand what you mean by onTextChange being called in a debounced fashion. Is there anything I can do to fix or mitigate the problem?

Sorry for the late reply

We debounce events from onTextChange as they could end up getting fired a lot. This is a platform feature and can't really be changed. Storing/processing every keystroke from input is not really possible today.

If you could explain the use case you are trying to solve here, maybe I can help you with an alternative solution @zzgvh

@hetunandu
Copy link
Member

@zzgvh were you able to find another way to solve this? Let me know if I can help.

@zzgvh
Copy link
Author

zzgvh commented Dec 2, 2021

Hey again @hetunandu. It seems the performance of Appsmith is a little better, particularly in deployed mode, but I still see problems when I do complex calls involving the store in event handlers, like Input.onTextChanged. Here's an example of a handler that can't keep up:

{{(
function updatePicksEditor1() {
  function doStore(key, value) {
    const editor = appsmith.store?.picks?.editor;
    storeValue("picks", {
      ...appsmith.store?.picks,
      editor: [
        {
          ...editor[0],
          [key]: value,
        },
        // spread operator for array literals do nothing if it is an empty array
        ...(editor[1] ? [editor[1]] : []),
      ],
    });
  }
  doStore("Sälj", Math.max(0, parseInt(SalesCount1.text || "0")));
}
)()}}

This code lives in onTextChange of an Input widget set to the Number type. If I hit the up or down arrows repeatedly there will be missed clicks.

slow_store.mp4

The vid shows the code above in action. First I go slow, four click up, this increases the value by four. Then four clicks down, value goes down as expected. Then I click up four times in quick succession. Only two register. Two more quick "four clicks" up. Two register each time. Then four clicks quickly down. They all go through. Finally four clicks up, only one of which registers.

I get the feeling that the events get mixed up time wise, so that early events actually fire after later ones. Just guessing of course, but if you look closely you can see the number go up and then down again when I hit up four times quickly.

I had hoped to be able to use the JS Editor to replace the store for these kinds of "page global data", by using the editor variables, but alas, that was not to be 😢 Seems like the JS editor variables cannot be changed and used by the page yet.

@hetunandu
Copy link
Member

hetunandu commented Dec 6, 2021

@zzgvh

In the first example I saw that you were writing and reading from the store in the same input. This, according to Appsmith is an antipattern.

Let's take an Input Widget for example. Features of the Input widget are:

  • Input widget has a text property that gets updated by the user typing in the input
  • The Input Widget will invoke onTextChange in a debounced fashion when ever the text changes
  • The Input Widget will update its text value whenever the defaultText value updates

The way you have set this input is such

  1. onTextChange updates the store
  2. the defaultText has the binding to the same store

So what ends up happening is this:
-> User updates the text input in quick succession (assume they are typing "1234")
-> The value is stored in the appsmith.store (since it is debounced only "12" went through)
-> the input's default text gets updated because the store has been updated ( updates to "12")
( meanwhile the user finishes typing "1234" )
-> the input's text gets updated with the default text (which was "12")
-> User lost the input as the new text property is "12"
-> if the user were to continue typing, it would append on "12" to become "1256" ("34" event seems to have been dropped)

And that is why, we consider this as an antipattern. In reactive systems like Appsmith, if two different sources update a single value at the same time, race conditions would occur. In this case, the two sources are the user typing and the widget defaultText setting the text.

I advice that you refer to the Input.text property directly to make decisions and business logic and avoid going through the store for this. We guarantee that Input.text will always be accurate when a user is typing in the input. defaultText is mainly used to reset the text property to an older or new state and not used to actively manage the state of the Input widget.

Hope I could explain this issue and the cause of it properly here. If not, I am happy to join you on a call to provide more examples. Incase you want me to help you with a workaround to your current problem, I can help with that as well.

@Nikhil-Nandagopal Nikhil-Nandagopal added Good First Issue Good for newcomers and removed Good First Issue Good for newcomers labels Dec 23, 2021
@hetunandu hetunandu removed their assignment Dec 24, 2021
@zzgvh
Copy link
Author

zzgvh commented Dec 30, 2021

Hey @hetunandu. Thanks for the extensive description of how the system "steps on its own toes" 😉 I think I now understand why this issue happens. So my question becomes, is there any other way that you can change the value of an input widget programmatically while allowing the user to use the same widget?

In the video, you see two buttons below the input, with a "-" and a "+", that I demo the problem on. When these buttons are pressed the appsmith.store value driving the defaultText of the input is changed by one. The UI reason for this is that when working on a tablet "fat fingers" make it hard to use the arrows of the input itself. Another use case that I have in another view is when I have two inputs that show values based on the same underlying appsmith store value. And so on; I have quite a few places where I cannot use just the raw value of an input, since I need to be able to change it via "external user action".

As it stands I cannot see how to make these kinds of somewhat involved uses of inputs without relying on the store for the value to be represented, and the onChanged event and defaultText property of the input to change and display the value. And it seems to me to be necessary part of being able to build sophisticated and (hopefully) user friendly functionality.

If you can devise some other way to accomplish this I'd be more than happy to "change my ways"! And if there is no other way I'd really like this to be an improvement of Appsmith in the future.

@Nikhil-Nandagopal
Copy link
Contributor

@zzgvh in this case, you can use the store to update the input's values but you don't need to write back to the store from the input when the input changes. I've detailed an example of this below. You can fork the app and check the code
https://app.appsmith.com/applications/61dfc1b919ba421fdd1ce508/pages/61dfc1b919ba421fdd1ce50b

@ajinkyakulkarni ajinkyakulkarni removed the High This issue blocks a user from building or impacts a lot of users label Feb 4, 2022
@ajinkyakulkarni ajinkyakulkarni added Low An issue that is neither critical nor breaks a user flow Needs More Info Needs additional information labels Feb 4, 2022
@bharath31
Copy link

assuming 30% of users who use storeValue face this issue

Stats

Stat Values
Reach 270
Effort (months) 0.5

@Nikhil-Nandagopal Nikhil-Nandagopal changed the title [Bug] Appsmith store drops updates when calls to storeValue() are made in quick succession [Bug]-[270]: Apr 12, 2023
@bharath31 bharath31 added Medium Issues that frustrate users due to poor UX and removed Needs More Info Needs additional information Low An issue that is neither critical nor breaks a user flow labels Apr 12, 2023
@bharath31 bharath31 self-assigned this Apr 12, 2023
@bharath31 bharath31 changed the title [Bug]-[270]: [Bug]-[270]: storeValue() drops values when called in quick succession Apr 12, 2023
@Nikhil-Nandagopal Nikhil-Nandagopal changed the title [Bug]-[270]: storeValue() drops values when called in quick succession [Bug]-[540]:storeValue() drops values when called in quick succession Feb 29, 2024
@Nikhil-Nandagopal Nikhil-Nandagopal added the Query & JS Pod Issues related to the query & JS Pod label Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Javascript Product Issues related to users writing javascript in appsmith JS Evaluation Issues related to JS evaluation on the platform Medium Issues that frustrate users due to poor UX Production Query & JS Pod Issues related to the query & JS Pod
Projects
None yet
Development

No branches or pull requests

7 participants