Skip to content

fix(ui): nested fields disappear when manipulating rows in form state #11906

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

Merged
merged 11 commits into from
Apr 1, 2025

Conversation

jacobsfletch
Copy link
Member

@jacobsfletch jacobsfletch commented Mar 28, 2025

Continuation of #11867. When rendering custom fields nested within arrays or blocks, such as the Lexical rich text editor which is treated as a custom field, these fields will sometimes disappear when form state requests are invoked sequentially. This is especially reproducible on slow networks.

This is different from the previous PR in that this issue is caused by adding rows back-to-back, whereas the previous issue was caused when adding a single row followed by a change to another field.

Here's a screen recording demonstrating the issue:

pr-11906.mp4

The problem is that requiresRender is never sent in the form state request for row 2. This is because the task queue processes tasks within a single useEffect. This forces React to batch the results of these tasks into a single rendering cycle. So if request 1 sets state that request 2 relies on, request 2 will never use that state since they'll execute within the same lifecycle.

Here's a play-by-play of the current behavior:

  1. The "add row" event is dispatched
    a. This sets requiresRender: true in form state
  2. A form state request is sent with requiresRender: true
  3. While that request is processing, another "add row" event is dispatched
    a. This sets requiresRender: true in form state
    b. This adds a form state request into the queue
  4. The initial form state request finishes
    a. This sets requiresRender: false in form state
  5. The next form state request that was queued up in 3b is sent with requiresRender: false
    a. THIS IS EXPECTED, BUT SHOULD ACTUALLY BE true!!

To fix this this, we need to ensure that the requiresRender property is persisted into the second request instead of overridden. To do this, we can add a new serverPropsToIgnore to form state which is read when the processing results from the server. So if requiresRender exists in serverPropsToIgnore, we do not merge it. This works because we actually mutate form state in between requests. So request 2 can read the results from request 1 without going through an additional rendering cycle.

Here's a play-by-play of the fix:

  1. The "add row" event is dispatched
    a. This sets requiresRender: true in form state
    b. This adds a task in the queue to mutate form state with requiresRender: true
  2. A form state request is sent with requiresRender: true
  3. While that request is processing, another "add row" event is dispatched
    a. This sets requiresRender: true in form state AND serverPropsToIgnore: [ "requiresRender" ]
    c. This adds a form state request into the queue
  4. The initial form state request finishes
    a. This returns requiresRender: false from the form state endpoint BUT IS IGNORED
  5. The next form state request that was queued up in 3c is sent with requiresRender: true

@jacobsfletch jacobsfletch changed the title fix(ui): removes object references when handling form state fix(ui): nested custom components sometimes disappear when manipulating rows in form state Mar 28, 2025
@jacobsfletch jacobsfletch changed the title fix(ui): nested custom components sometimes disappear when manipulating rows in form state fix(ui): nested fields disappear when manipulating rows in form state Mar 28, 2025
@jacobsfletch jacobsfletch marked this pull request as ready for review March 28, 2025 21:50
@jmikrut
Copy link
Member

jmikrut commented Mar 31, 2025

New approach: remove requiresRender COMPLETELY 😈

Right now, we need requiresRender as an instruction for buildFormState to determine if we should render fields or not. Fields have this property set on each, or forceRender can be set to render all fields on first document load. If we want to remove requiresRender, we need to reliably determine when a field should be rendered / re-rendered on the server.

How to determine if the field needs to be rendered

Right now, we omit the customComponents property from field state when we send from client to server. This makes it so we don't know if a field has been rendered or not solely on the server, because we receive no information about the field rendered state.

We will try and add a new prop called:

lastRenderedPath: 'my-path' and send that from client to server for all fields.

This way, we only need to check that certain fields have lastRenderedPath - if they don't, then it needs to be rendered. If the lastRenderedPath is different from the path, then we need to re-render.

New steps for when building a field state:

  1. Does this field have any custom components? If yes, proceed
  2. Is this a rich text field? if yes, proceed
  3. Does this field not have lastRenderedPath? OR, does lastRenderedPath differ from the field's path?
  • If either are yes, render!

The above flow appears to allow us to completely remove the requiresRender form state property.

Work to do prior to implementing this new approach:

  • Take an inventory of where we use this prop right now
  • Make sure Lexical can still accomplish what it needs without any reference to requiresRender. If you want to force a field to re-render, you could always remove the lastRenderedPath prop from a field's state. Then its custom components would re-render.

Branch: https://github.com/payloadcms/payload/tree/fix/component-field-state

@jmikrut jmikrut merged commit 373f6d1 into main Apr 1, 2025
76 checks passed
@jmikrut jmikrut deleted the fix/form-state-mutations branch April 1, 2025 13:54
Copy link
Contributor

github-actions bot commented Apr 1, 2025

🚀 This is included in version v3.32.0

Bjornnyborg pushed a commit to Bjornnyborg/payload that referenced this pull request Apr 16, 2025
…payloadcms#11906)

Continuation of payloadcms#11867. When rendering custom fields nested within
arrays or blocks, such as the Lexical rich text editor which is treated
as a custom field, these fields will sometimes disappear when form state
requests are invoked sequentially. This is especially reproducible on
slow networks.

This is different from the previous PR in that this issue is caused by
adding _rows_ back-to-back, whereas the previous issue was caused when
adding a single row followed by a change to another field.

Here's a screen recording demonstrating the issue:


https://github.com/user-attachments/assets/5ecfa9ec-b747-49ed-8618-df282e64519d

The problem is that `requiresRender` is never sent in the form state
request for row 2. This is because the [task
queue](payloadcms#11579) processes tasks
within a single `useEffect`. This forces React to batch the results of
these tasks into a single rendering cycle. So if request 1 sets state
that request 2 relies on, request 2 will never use that state since
they'll execute within the same lifecycle.

Here's a play-by-play of the current behavior:

1. The "add row" event is dispatched
    a. This sets `requiresRender: true` in form state
1. A form state request is sent with `requiresRender: true`
1. While that request is processing, another "add row" event is
dispatched
    a. This sets `requiresRender: true` in form state
    b. This adds a form state request into the queue
1. The initial form state request finishes
    a. This sets `requiresRender: false` in form state
1. The next form state request that was queued up in 3b is sent with
`requiresRender: false`
    a. THIS IS EXPECTED, BUT SHOULD ACTUALLY BE `true`!!

To fix this this, we need to ensure that the `requiresRender` property
is persisted into the second request instead of overridden. To do this,
we can add a new `serverPropsToIgnore` to form state which is read when
the processing results from the server. So if `requiresRender` exists in
`serverPropsToIgnore`, we do not merge it. This works because we
actually mutate form state in between requests. So request 2 can read
the results from request 1 without going through an additional rendering
cycle.

Here's a play-by-play of the fix:

1. The "add row" event is dispatched
    a. This sets `requiresRender: true` in form state
b. This adds a task in the queue to mutate form state with
`requiresRender: true`
1. A form state request is sent with `requiresRender: true`
1. While that request is processing, another "add row" event is
dispatched
a. This sets `requiresRender: true` in form state AND
`serverPropsToIgnore: [ "requiresRender" ]`
    c. This adds a form state request into the queue
1. The initial form state request finishes
a. This returns `requiresRender: false` from the form state endpoint BUT
IS IGNORED
1. The next form state request that was queued up in 3c is sent with
`requiresRender: true`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants