frontend: use stable keys for channel and version lists - #1557
Open
archiik04 wants to merge 1 commit into
Open
Conversation
Issue flatcar#328 asked whether each use of an array index as a React key is intended and safe. Three of the ten occurrences had a stable identifier already in scope, so they now key on that instead: - VersionBreakdownBar keyed <Bar> by index while dataKey was already the version string. - Packages/Item now keys channel labels on channel.id. - ApplicationItemChannelsList mapped channels twice and keyed both by index. Collapsing to a single map allows keying on channel.id, and drops an "if (channels)" branch that was always true because channels is defaulted to an empty array above it. The rest are left alone. Tabs uses the index as the identity and passes it to a11yProps. ListHeader, MoreMenu and Instances/Charts render static lists that do not reorder. TimelineChart is also index-keyed but is in scope for flatcar#407. SimpleTable keys rows by index over paginated data, which is the one place this could actually mis-associate rows. Its instances prop is untyped and neither caller supplies an id, so a correct fix needs a derived key or a new prop; that is left for a separate change. No behaviour changes today, since none of these lists currently reorder. Refs flatcar#328 Signed-off-by: Archi Kanungo <archi.kanungo2004@gmail.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Issue #328 asked whether each use of an array index as a React key is intended and safe. I went through all ten occurrences. Three had a stable identifier already in scope and now key on it:
VersionBreakdownBarkeyed<Bar>by index whiledataKeywas already the version string.Packages/Itemnow keys channel labels onchannel.id.ApplicationItemChannelsListmapped channels twice and keyed both by index. The second map only received aReactNode, so it had no access to the channel; collapsing to a single map is what makes keying onchannel.idpossible. That also drops anif (channels)branch which was always true, sincechannelsis defaulted to[]above it.I left the rest alone:
Tabs: the index is the identity. It is also passed asindexand toa11yProps.ListHeader,MoreMenu,Instances/Charts: static lists that never reorder.TimelineChart: also index-keyed, but it is in scope for Revisit chart animation on extended group view #407, which someone else is already working on. I stayed out of it.SimpleTableis the one place where this could actually mis-associate rows, since it keys rows by index over paginated data. Itsinstancesprop is typedany[]and neither caller supplies an id, so a correct fix needs either a key derived from the row's column values (which assumes no two rows are ever identical) or a newgetRowKeyprop. Both felt like a separate discussion, so I kept it out. Happy to take it on if you have a preference. I've usedRefs #328rather thanFixesfor that reason.Testing
npx tsc -b,npx eslint --max-warnings 0, andnpx vitest run(79 tests, 20 files) all pass.I did not add tests. None of the existing tests would have failed before this change either, since these lists do not currently reorder and there is no observable behaviour change today. A test that proved key stability would have to assert DOM node identity across a reorder, which seemed more brittle than valuable. If you'd rather have one, say so and I'll add it.