form icon indicating copy to clipboard operation
form copied to clipboard

Moving subfields of array field mix their values

Open pedro757 opened this issue 9 months ago • 5 comments

Describe the bug

When the input is still idle (I mean undefined) and you try to change its position it takes the value of other subfields. It messes things up.

Codesandbox

https://codesandbox.io/p/sandbox/focused-pond-s96spf?file=%2Fsrc%2FApp.js%3A169%2C8

Steps to reproduce

  1. Try to move the empty subfields dragging and dropping in other position
  2. Now try to move subfields with non-empty values

Expected behavior

I expect it to keep its value.

pedro757 avatar Apr 27 '24 14:04 pedro757

I think this is a bug with our moveValue method. I'll investigate in a bit

crutchcorn avatar Apr 29 '24 18:04 crutchcorn

I think this is a bug with our moveValue method. I'll investigate in a bit

If it saves a little time, after some research, I wrote a test to check whether there was a problem with the moveValue function, but the test went well.

I wonder if the problem isn't the collision between an uncontrolled and a controlled field, and that screws up the ui.

  it('should correctly move an array value, even with undefined values', () => {
    const form = new FormApi({
      defaultValues: {
        users: [
          {
            name: undefined,
          },
          {
            name: 'one',
          },
          { name: 'two' },
          {
            name: 'three',
          },
          {
            name: undefined,
          },
        ],
      },
    })

    const field = new FieldApi({
      form,
      name: 'users',
    })

    field.moveValue(3, 4)

    expect(field.getValue()).toStrictEqual([
      {
        name: undefined,
      },
      { name: 'one' },
      { name: 'two' },
      {
        name: undefined,
      },
      {
        name: 'three',
      },
    ])
  })

t-rosa avatar Jun 22 '24 17:06 t-rosa

Hey @Balastrong, was this fixed when you updated your move and insert stuff?

crutchcorn avatar Jun 22 '24 17:06 crutchcorn

Unfortunately I think there's still something not behaving here. I made a short example: https://stackblitz.com/edit/tanstack-form-grdmqr?file=src%2Findex.tsx&preset=node

First issue is that indeed having <input value={subField.state.value} ... /> can't work if value changes from string to undefined and vice-versa when moving values, but this may be on React and subField.state.value ?? '' can help.

The second issue is that if we use the move button and then we submit, the old value "one" remains in the input, but again this is probably still related to undefined being put there.

Balastrong avatar Jun 23 '24 12:06 Balastrong

This test on react-form fails because magically the second inputs gets back the value 'one' on submit. The same operation (move then submit) on form-core passes.

Failing Test

it('should correctly move an array value, even with undefined values and not alter the state after submit', async () => {
    function Comp() {
      const form = useForm({
        defaultValues: {
          people: [
            {
              name: undefined,
            },
            {
              name: 'one',
            },
          ],
        },
      })

      return (
        <div>
          <form
            onSubmit={(e) => {
              e.preventDefault()
              e.stopPropagation()
              form.handleSubmit()
            }}
          >
            <form.Field name="people" mode="array">
              {(field) => {
                return (
                  <div>
                    {field.state.value.map((_, i) => {
                      return (
                        <form.Field key={i} name={`people[${i}].name`}>
                          {(subField) => {
                            return (
                              <div>
                                <label>
                                  <div>Name for person {i}</div>
                                  <input
                                    value={subField.state.value ?? ''}
                                    onChange={(e) =>
                                      subField.handleChange(e.target.value)
                                    }
                                  />
                                </label>
                              </div>
                            )
                          }}
                        </form.Field>
                      )
                    })}
                    <button type="button" onClick={() => field.moveValue(1, 0)}>
                      Move 1 to 0
                    </button>
                  </div>
                )
              }}
            </form.Field>
            <form.Subscribe
              selector={(state) => [state.canSubmit, state.isSubmitting]}
              children={([canSubmit, isSubmitting]) => (
                <>
                  <button type="submit" disabled={!canSubmit}>
                    {isSubmitting ? '...' : 'Submit'}
                  </button>
                </>
              )}
            />
          </form>
        </div>
      )
    }

    const { getByText, findByLabelText, queryByText, findByText } = render(
      <Comp />,
    )

    const input = await findByLabelText('Name for person 0')
    expect(input).toBeInTheDocument()
    expect(input).toHaveValue('')

    const input2 = await findByLabelText('Name for person 1')
    expect(input2).toBeInTheDocument()
    expect(input2).toHaveValue('one')

    await user.click(getByText('Move 1 to 0'))

    expect(input).toHaveValue('one')
    expect(input2).toHaveValue('')

    await user.click(await findByText('Submit'))

    expect(input).toHaveValue('one')
    expect(input2).toHaveValue('') // This gets value 'one'
  })

Balastrong avatar Jun 23 '24 13:06 Balastrong