It looks like in our case we can use this approach: https://robinpokorny.medium.com/index-as-a-key-is-an-anti-pattern-e0349aece318

the list and items are static–they are not computed and do not change;

We call pushModal and never update what's inside

the items in the list have no ids;

We would need to introduce some kind of id. It sounds like the context could determine it. Not sure if React allows Symbols as keys, but that would be nice.

the list is never reordered or filtered.

That's the case.

So overall, I think we should check if there's a way to generate unique ids and use them as keys. Even though our current solution should work well, it's less maintainable as it is possible to violate the conditions in the future. But if we can't find a good solution with other keys, we can keep the idx.

We don't need to destructure - slice doesn't modify the original array

We don't need to have optional node here, right?

We usually don't use jsx in components that fetch the data (so in this case all the jsx would be in App). But I also don't think there's anything wrong with doing what you've done - so we can probably keep it.

I'm not sure but it sounds like all the uses of clearModal should be replaced by popModal unless there's a good reason to use clearModals. What do you think?

popModal?

This is an interesting issue. If a user clicks outside, do we want to close all the modals or only the one from the top? I'd say that we should close only one.

I guess pushModal is always present

We probably don't need this invariant

Is it really necessary to support calling this function with null?

If it is, then we can generate uuid in the if (or add if(!newModal) { return; } block)

Yes, thanks for catching it

Agree. This shouldn't make any difference and with all current clearModal usages, both popModal and clearModals should work exactly the same. The popModal however seems to be a bit closer to currently existing intention, which was to clear one, currently displayed modal.

I'll replace clearModal with popModal

Agree. It's probably the best to close only the one on the top

I suppose it is. I don't know why this condition was placed here, and I think it may be safely removed

The old setModal had ?React.Node parameter. I don't think it's necessary, so I'll remove the optional

This variable was necessary only because of the if. I think we can just have onClick={this.onClick} in the return