-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
lib: Fix DynamicListForm state confusion on removals #19825
Conversation
We name all other custom Cockpit components in that form.
Drop the unnecessary list copies in setState() callbacks. `onChange()` is not supposed to modify its argument. Use a spread for making a list copy in removeItem(), and creating the new `list` entry in addItem().
When deleting an item, set it to `undefined` (i.e. keep it as a hole in `list`) instead of completely removing it. Before, removing the first item shifted all the indexes around, which broke keys/ids and the state handling, so that it appeared as if the wrong element got deleted. With that, `list.length` becomes meaningless. Adjust the emptiness check in the render function. Entirely drop `itemCount` -- nothing uses that, and it's a bit expensive to compute, so let's only introduce this when there is an actual use case. https://issues.redhat.com/browse/RHEL-19598
Works, first run in cockpit-project/cockpit-machines#1381 is green. |
Podman also uses this component, as the TF tests are green this should introduce no issues for cockpit-podman right? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! That was a very subtle bug which we never caught in cockpit-podman!
@jelly this is a build-time change, runtime is okay. I'll send a podman PR similar to cockpit-project/cockpit-machines#1381 afterwards to mop up the API change. But I did confirm that podman also doesn't use the |
When deleting an item, set it to
undefined
(i.e. keep it as a hole inlist
) instead of completely removing it. Before, removing the firstitem shifted all the indexes around, which broke keys/ids and the state
handling, so that it appeared as if the wrong element got deleted.
With that,
list.length
becomes meaningless. Adjust the emptiness checkin the render function. Entirely drop
itemCount
-- nothing uses that,and it's a bit expensive to compute, so let's only introduce this when
there is an actual use case.
https://issues.redhat.com/browse/RHEL-19598
no-test, as we don't use this component anywhere in cockpit. This is for cockpit-machines. cockpit-project/cockpit-machines#1381 adds an integration test for that bug, and runs against that branch for the time. I tested it locally interactively, but I want to see green machines tests before we land this.