fix: ui.text_field value not changing with use_state

[AkshatJawne]

Resolves #372

Changes Implemented:

• Added a useEffect that updates the local value state in the TextField whenever the propValue changes, which is the value that would be changed using the use_state

[mofojed]

It works, but needs cleanup - use the usePrevious hook instead, and don't fallback to default_value (shouldn't need to).

I tested with a few different components:

# Controlled input with a reset button
@ui.component
def my_input():
    text, set_text = use_state("hello")
 
    def handle_stage_submit():
        set_text("goodbye")

    return [ui.flex(
        ui.text_field(value=text, on_change=set_text),
        ui.text(f"You typed {text}"),
        direction="column",
    ), ui.flex(ui.action_button("Submit", on_press=handle_stage_submit))]
 

mi = my_input()

# Controlled input, just setting the value
@ui.component
def my_input2():
    text, set_text = use_state("hello")

    return [ui.flex(
        ui.text_field(value=text, on_change=set_text),
        ui.text(f"You typed {text}"),
        direction="column",
    )]
 

mi2 = my_input2()

# Uncontrolled input, only setting the default value
@ui.component
def my_input3():
    text, set_text = use_state("hello")

    return [ui.flex(
        ui.text_field(default_value=text, on_change=set_text),
        ui.text(f"You typed {text}"),
        direction="column",
    )]
 

mi3 = my_input3()

[mofojed]

My bad, you do actually need the pending stuff as @mattrunyon called out.

Test with a widget like:

from deephaven import ui
from time import sleep


# Controlled input, just setting the value
@ui.component
def my_slow_input():
    text, set_text = use_state("hello")

    def sleep_set_text(new_text: str):
        sleep(2)
        set_text(new_text)

    return [ui.flex(
        ui.text_field(value=text, on_change=sleep_set_text),
        ui.text(f"You typed {text}"),
        direction="column",
    )]
 

msi = my_slow_input()

But you should wait for the result of the call to onChange that's passed in to TextField before resetting the pending state. So a couple of things around that:

• We want to set pending state to true immediately in onChange (i.e. not after a debounce)
• We don't want to reset that pending state to false until after we await/receive the response from the call to propOnChange (which will be after debounce, and after the server has received/responded to the request)
  • We'll need to update the type of onChange passed in as a prop to return Promise<void> instead of just void (by extending the DHCTextFieldProps type)

Then I think we should be good. This case would be much more of an issue in environments were the server is remote and there is some lag, so it's not as exposed if you're running localhost unless you call sleep to simulate it as in the example above.

[AkshatJawne]

@mofojed @mattrunyon Attempted to implement code based on comments, have the following code, but after testing, it appears to break the functionality. Not sure if I am misunderstanding?

...
  const [value, setValue] = useState(propValue ?? defaultValue);
  const [pending, setPending] = useState(false);
  const prevPropValue = usePrevious(propValue);

  if (
    propValue !== prevPropValue &&
    propValue !== value &&
    propValue !== undefined &&
    !pending
  ) {
    setValue(propValue);
  }

  const debouncedOnChange = useDebouncedCallback(async (newValue: string) => {
    await propOnChange(newValue);
  }, VALUE_CHANGE_DEBOUNCE);

  const onChange = useCallback(
    (newValue: string) => {
      setPending(true);
      setValue(newValue);
      debouncedOnChange(newValue);
      setPending(false);
    },
    [debouncedOnChange]
  );
...

[AkshatJawne]

BTW: I did extend the props as mentioned, just do not have it inlcuded in the pasted snippet. I suspect there is an issue with the setting of the pending state, but I believe it is in alignment with how @mofojed mentioned in his last comment

[mattrunyon]

The debouncedOnChange is async, so you'd want to set isPending to false after the await in that function.

What's happening right now is you set pending, then fire the debounced function, and immediately set pending to false right after.

I think you could also use a ref instead of state here. Not sure if it will make much difference, but it would prevent a re-render after the debounced function returns since the pending state does not affect rendering. The server will push new props if the callback triggers a prop change on the server side

[AkshatJawne]

Hmm interesting, I had tried that but it does seem to change the problem. The text in the "You typed" string still does not appear to change.

I will try using a ref instead of a state though, maybe that might fix the issue.

[mattrunyon]

Another edge case that maybe we don't care about unless someone complains about it is you could have a pending onChange and a reset button. Clicking the reset button while the change is pending could result in the reset not working

[AkshatJawne]

Need @mattrunyon 's approval to merge

[mattrunyon]

Do we have any examples that can trigger this? I'm not sure if we want to swallow this or show it to the user (or if the existing error handling would show it to the user)

[AkshatJawne]

There could be cases where the onChange passed in could be faulty or possess an error, in that case, we can catch that here. @mofojed, were there was any specific cases you had in mind when suggesting this?

[mofojed]

Should be able to test with something like:

from deephaven import ui

def raise_error(v):
    raise ValueError("Boom")

tf = ui.text_field(on_change=raise_error)

Previously it would log an error in the python console, and an uncaught exception in the browser console. Now it's a warning in the browser console. It still logs in the python console as an error (which I think is correct).

We could potentially report an error on the TextField itself, by setting errorMessage and validationState, but that's out of scope for this ticket and also doesn't necessarily make sense. Either way would log that as a separate ticket.

[mattrunyon]

Ok that's fair. I'm not sure if the component should blow up like if the error was raised outside of the change handler. Right now if this component were in a dashboard there would be no indication to the user (without log monitoring) that something went wrong.

[mattrunyon]

If we need this in other inputs, I would recommend creating a helper function or hook to handle this case (in a separate PR if adding to other input types). That way the logic is not duplicated