lib: Fix DynamicListForm state confusion on removals #19825
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
martinpitt
added
blocked
1 task
martinpitt
removed
the
blocked
|
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? |
jelly
approved these changes
Jan 9, 2024
|
@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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.lengthbecomes 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.