fix(fuselage-ui-kit): Initial Value being ignored on text input fields upon modal update

[murtaza98]

• I have read the Contributing Guide
• I have signed the CLA
• Lint and unit tests pass locally with my changes
• I have labeled the PR correctly with the related package
• I have run Loki's visual regression tests (if applicable)
• I have added tests that prove my fix is effective or that my feature works (if applicable)
• I have added necessary documentation (if applicable)
• Any dependent changes have been merged and published in downstream modules

Proposed changes (including videos or screenshots)

Issue(s)

When the app tries to update the modal with a new input field, the value from the previous input field still persists in the new input field - thus ignoring the initialValue param being passed from the app.

Note this only occurs if the Input field is added via view-update (i.e via modify.getUiController().updateModalView()).
Please refer to the following demo for more info about this bug.

Also, the code for this sample app is available here. To test, simply install the app, and run /multi-form create command and replicate the issue as shown in the video

multi-form-issue.mp4

Further comments

[d-gubert]

I think the behavior you mentioned might be intended, even if it's unclear. Because when you update a view, we assume that you're maintaining context (semantically speaking), only adjusting the presentation - so what is there should be kept; and when you open a new modal view (modals can be stacked up to 3 times IIRC) then you're changing the context completely, which allows for a totally new state.

In the example you provided, for instance, ideally you should not be updating your view, but rather opening a new modal, since the context has completely changed.

A possible side-effect of removing this behavior would be breaking the Poll app, since it relies on UIKit keeping the state of the form when you click the "Add a choice" button. See image:

@sampaiodiego @ggazzo wdyt?

[murtaza98]

Hi @d-gubert Firstly thanks for checking out this PR 😄

Because when you update a view, we assume that you're maintaining context (semantically speaking), only adjusting the presentation - so what is there should be kept;

I totally agree with this point, if an input field has a value set by the user, then it should always maintain that value and the initialState prop shouldn't be allowed to change that. But this is not what's happening here... here there are 2 different inputs with 2 completely different actionIds, and the state from the first input is getting copied to the inititalState of the second input which I don't feel is a good idea since both of them are 2 unrelated inputs.

Moreover, I wasn't able to replicate the poll app issue which you mentioned with the poll app. Could you please share some more details so I can reproduce this issue and check what's wrong here?

poll-app-working-with-initial-state-fix.mp4

On a side note: There's one more use-case I'd like to share here which is getting affected by this bug (this is a live one with a valid use-case). Try installing the Poll Plus app and start a live question survey using /poll live 2 - this would allow you to add 2 questions simultaneously to a single poll. Now here, notice when you move from the first question to the second question, the values get prepopulated within the 2nd question from the first question... which I feel really is confusing to the end-user

poll-plus-app-live-question-issue-with-initialstate.mp4

[d-gubert]

But this is not what's happening here... here there are 2 different inputs with 2 completely different actionIds, and the state from the first input is getting copied to the inititalState of the second input which I don't feel is a good idea since both of them are 2 unrelated inputs.

Oh, I see! Yes, this makes sense.

Moreover, I wasn't able to replicate the poll app issue which you mentioned with the poll app.

Interesting, by the changes you've applied to the code I'd assume that would happen (I haven't tested it). But upon a closer look I think I see how it didn't. That's why we need a review from the Frontend team 😅

[ggazzo]

I need to check, has been a long time since I wrote this, but your solution doesn look good, if every single time I change _value I set value why not just use _value as value instead? there are a lot of code there to make things happen as we are used (right or not) your useEffect basically ignores/removes the utility of 90% from the rest of the code.

[ggazzo]

thanks for bringing this issue to us
@murtaza98 please check #601 if solves your issue
3 items to point:

• I'm not sure if the app is only for tests, but you should avoid navigate using actions, the modal has a proper way to add steps
• we use conventional commits at this repository
• this is a quite common "problem" on react, thats why they dont recommend using index as key (if we can avoid that), using index, the both elements were key 0 and there were no way to react identify/destroy/create a new one with initial value, your solution, although working (in practice) goes against some concepts of react. but this a good example to never forget about keys + react
  thanks again ;)

[murtaza98]

@ggazzo Thank you so much for looking into this issue and adding a proper fix. It indeed solves the issue 🚀

PS: While testing locally I found a minor issue with the Poll app message (specifically with the "Finish Poll" button) and added a fix for it. This was the error I was getting. It was just a minor null check issue 😉