Table: Multi-select bug which removes selected rows when model updates (HDS-4273)

[KristinLBradley]

📌 Summary

If merged, this PR fixes the issue with selectableRows state not being maintained properly after model update.

(NOTE: Initial commits include test code from Michael Lange which will be removed before merge.)

👉 See related PR to update docs for multi select table used with a model.

🛠️ Detailed description

TODO: Add/update related tests.

PREVIEW: https://hds-showcase-git-hds-4273-table-multi-select-bug-hashicorp.vercel.app/components/table-multi-select-bug

Bug: Originally, clicking "Force new data" button in the first example wiped out the selectableRows array due to timing of didInsert & willDestroy calls.

Expected: SelectableRows array should have the proper set of checkbox items after model update and willDestroy calls finish.

🔗 External links

• Jira ticket: HDS-4273

---

👀 Component checklist

• Percy was checked for any visual regression
• A changelog entry was added via Changesets if needed (see templates here)

💬 Please consider using conventional comments when reviewing this PR.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
hds-showcase	✅ Ready (Inspect)	Visit Preview	Jan 17, 2025 11:04pm
hds-website	✅ Ready (Inspect)	Visit Preview	Jan 17, 2025 11:04pm

[KristinLBradley]

This fixes what seems to be the actual problem. Would appreciate feedback though. (I'll clean up all the messy commented out & test code later.)

[KristinLBradley]

Thanks, I updated the code.

I was wondering though if it's an issue if the new model has a different number of rows or different keys or something. I added another example to the Showcase page to test this and it doesn't work correctly but I don't know if it's an actual realistic concern.

[didoo]

@KristinLBradley I added a few thoughts/comments, but I want to double check with @DingoEatingFuzz if I'm understanding correctly the problem (I may be missing something).

[didoo]

@KristinLBradley @alex-ju I had a quick call with @DingoEatingFuzz (thanks 🙏) to see if I was understanding correctly the problem and if the solution(s) proposed are be what he would expect to see, as a "consumer" of HDS.

A few points below, to summarize what we discussed:

• the solution(s) proposed in this PR (be it to remove only the "old" row from the _selectableRows on willDestroyRowCheckbox, or to remove the duplicate row in the didInsertRowCheckbox) will solve the main problem, which is having the "select all" checkbox in an unusable state; but the problem that the checkbox status is lost when the model is updated remains
• using the @identityKey argument solves this problem, but Michael (as consumer) didn't know that he had to add the @identityKey for this, and it took him some time to figure it out, and that's not great
• he would prefer to see the HDS component take care of this, but (personally) I am not sure how to do it without something that would amount to a refactoring of the internals/logic of the multi-select table; I'll get back to this in a sec
• we have also checked how many instances of the selectable table there are (not many) so this is more of a special case (the one in which consumers have to add the identityKey to fix something that doesn't work as expected)

I'll leave it to him chime in, in case I've forgotten/misinterpreted something, or if he wants to add more context or his point og view as HDS "consumer".

[didoo]

Re. the problem of the loss of the checkbox state when the model is updated but there is no @identityKey specified (so it defaults to @identity) one option I've tried is to just recycle the existing item in the "_selectableRows", but even in that case Ember re-renders the rows (exactly because it sees them as new "rows" in the model, so it's not a viable option to preserve the state without having an @identityKey.

[KristinLBradley]

@KristinLBradley @alex-ju + cc @DingoEatingFuzz at this point I think we have limited options in front of us:

• refactor the component, to avoid "reading" the state from the checkbox, but having an internal proxy for it [big job]

• fix "just" the problem of the select all checkbox getting in the messy state by

  • remove only the "old" row from the _selectableRows on willDestroyRowCheckbox
  • remove the duplicate row in the didInsertRowCheckbox (my preferred solution)

Thoughts? Alternatives?

@didoo @alex-ju + cc @DingoEatingFuzz

Removing the duplicate row seems to work as long as there is a one to one correspondance between the new rows and the old rows. But it breaks if there are a different number of rows and/or key changes. So can we be certain that the rows won't change between model updates?

[KristinLBradley]

@KristinLBradley @alex-ju I had a quick call with @DingoEatingFuzz (thanks 🙏) to see if I was understanding correctly the problem and if the solution(s) proposed are be what he would expect to see, as a "consumer" of HDS.

A few points below, to summarize what we discussed:

• the solution(s) proposed in this PR (be it to remove only the "old" row from the _selectableRows on willDestroyRowCheckbox, or to remove the duplicate row in the didInsertRowCheckbox) will solve the main problem, which is having the "select all" checkbox in an unusable state; but the problem that the checkbox status is lost when the model is updated remains
• using the @identityKey argument solves this problem, but Michael (as consumer) didn't know that he had to add the @identityKey for this, and it took him some time to figure it out, and that's not great
• he would prefer to see the HDS component take care of this, but (personally) I am not sure how to do it without something that would amount to a refactoring of the internals/logic of the multi-select table; I'll get back to this in a sec
• we have also checked how many instances of the selectable table there are (not many) so this is more of a special case (the one in which consumers have to add the identityKey to fix something that doesn't work as expected)

I'll leave it to him chime in, in case I've forgotten/misinterpreted something, or if he wants to add more context or his point og view as HDS "consumer".

@didoo In addition to the current code changes, we could also update the documentation to mention that if you want the state of the checkboxes to persist after the model updates, you'll need to add an identityKey.

Do you think this the current changes together with updating the docs would be enough to address the main problem?

[didoo]

Removing the duplicate row seems to work as long as there is a one to one correspondance between the new rows and the old rows. But it breaks if there are a different number of rows and/or key changes. So can we be certain that the rows won't change between model updates?

@KristinLBradley I've played quite some time with the code yesterday, and I wasn't able to find an OK solution. The root of the problem is the timing of did-insert vs will-destroy (the first one is deterministic, the second one is not). The solution that you have found works "by chance" (and the one I proposed works only in certain cases) but I personally think both are not "the right one" from a code logic perspective. If you want I can discuss more (maybe in a pairing session) or I can have a look and propose something when we're back from the offsite. In the meantime I would suggest to put on hold on this task (apart from the documentation update, see below).

@didoo In addition to the current code changes, we could also update the documentation to mention that if you want the state of the checkboxes to persist after the model updates, you'll need to add an identityKey.

Yes, this is something that I would do anyway, independently from this ticket/task, that is meant to fix the case for when the consumer is not adding the identityKey (even if technically is not a fix, it's a more graceful failure to maintain the state)

Do you think this the current changes together with updating the docs would be enough to address the main problem?

See my comments above.

/cc @alex-ju @DingoEatingFuzz

[KristinLBradley]

@didoo In addition to the current code changes, we could also update the documentation to mention that if you want the state of the checkboxes to persist after the model updates, you'll need to add an identityKey.

Yes, this is something that I would do anyway, independently from this ticket/task, that is meant to fix the case for when the consumer is not adding the identityKey (even if technically is not a fix, it's a more graceful failure to maintain the state)

@didoo

How is this? I opened a PR: #2667