fix: unique key warning in FrequentlyViewedTab component

[OnkarRuikar]

Summary

The FrequentlyViewedTab gives warning:

Each child in a list should have a unique "key" prop

Solution

Give each FrequentlyViewedEntry an unique id. This will be helpful when the sort feature implementation gets completed.

How did you test this change?

On Firefox and Chromium on Fedora 36.

[OnkarRuikar]

Signed the commit.

[OnkarRuikar]

The rebase wiped out all the old un-staged files; it took some time to fake 'MDN Plus' in dev environment again. 😭

To support the sort feature we do need to have consistent/permanent ids for the entries. We have to store the ids in local storage as part of the object.

---

The lazy evaluation of entry || index++ is dangerous

It was done to avoid duplicate ids with existing entries that users have in their localstorage at the moment. Due to the way usePersistFrequentlyViewed method is implemented(the sorting on views), it would've lead to duplicate ids if we had used index of the entry. Consider following scenario if we use entry's index:

• A user has 3 entries at the moment: {name: a, views: 3}, {name: b, views:2}, and {name: c, views:1}.
• Note none of them have ids. We are introducing the id in the PR.
• After the PR gets merged the client side sees the new code.
• User views document 'd' say 4 times. The entry for it will be {name:d, id: 3, views:4}.
• Due to the sorting, the local storage will have entries like [ {name:d, id:3, views:4}, {name:a, views:3}...]
• User opens the 'Frequently viewed page'
• Our method will generate ids like this. [ {name:d, id:3, views:4}, {name:a, id:1, views:3}, {name:b, id:2, views:2}, {name:c, id:3, views:1}]
• Note, now 'd' and 'c' got same ids.

---

Above doesn't matter anymore because I've changed the solution. Explanation for the new solution:

Using index for id:

 const newEntry: FrequentlyViewedEntry = {
      id: frequentlyViewed.length,          // <--- bad idea
      url: doc.mdn_url,
      title: doc.title,
      parents: doc.parents,
      timestamp: new Date().getTime(),
      visitCount: 1,
    };

is a bad idea. Because we let user delete entries. Suppose there are 3 entries at the moment. The next entry gets id 3. Then user deletes first entry and adds a new one. The last entry again will get id 3. So we'll end up with 3rd and 4th entries with id 3.

The new solution relies on the fact that we allow only 20 entries at any moment: FREQUENTLY_VIEWED_MAX_ITEMS. So we allot ids between 0-20. The deleted ids will be reused:

function getNextUniqueID(entries: FrequentlyViewedEntry[]): number {
  const ids = new Array(FREQUENTLY_VIEWED_MAX_ITEMS + 1);
  for (const entry of entries) {
    if (entry.id !== undefined) {
      ids[entry.id] = true;
    }
  }
  return ids.findIndex((id) => id === undefined);
}

I chose to use array cos there are only 20 numbers to choose from. Without the array we would need at worse 400(0-19 x 20 items) iterations.

---

About the change in file collection-list-item.tsx. The component is being reused for both BookmarkData and FrequentlyViewedEntry types. The bookmarks have extra edit feature. They used id to distinguish between the two types: {"id" in item && onEditSubmit. But now we have the id on FrequentlyViewedEntry as well. :O So, I chose notes for the distinction: {"notes" in item && onEditSubmit

Ping me on element/matrix chat if required.

[github-actions[bot]]

This pull request has merge conflicts that must be resolved before it can be merged.

[OnkarRuikar]

I've rebased the branch. Looks like some refactoring happened around types.ts.

[OnkarRuikar]

After rebase on GitHub my commits became un-verified!? :angry:

This is the same issue as https://github.com/community/community/discussions/12613 What can be done to fix this and avoid this?

[caugner]

@OnkarRuikar Since this has happened a lot recently, I have now disabled the button ("Always suggest updating pull request branches"). In this PR, you could simply squash all your commits together (using git rebase --interactive).

Or you could try git rebase --interactive --exec "git commit --amend --no-edit -S", but I'm not sure if that works.

[OnkarRuikar]

@caugner Thanks! Since there were latest commits available I did git rebase main and force pushed it. It's good to go now.

[OnkarRuikar]

Unfortunately older items don't seem to actually be updated in the Local Storage with an index.

Good catch! This is because we didn't update them in usePersistFrequentlyViewed function. I've committed the required change.

Before fix

On main branch:

After fix

On this feature branch:

Note: before running the fix I visited Learn/Accessibility/HTML. So there were 3 entries in the storage. After deploying the fix I visited Learn/CSS/Building_blocks/A_cool_looking_box and all the entries got their indexes stored in the local storage.

[github-actions[bot]]

This pull request has merge conflicts that must be resolved before it can be merged.

[caugner]

@OnkarRuikar I merged latest main into the branch, including the conflicting changes from https://github.com/mdn/yari/pull/6352, which actually allowed us to deduplicate the backfill of old entries.

Could you please have a last look if the current state of the PR looks good to you? Then I would merge it next.

[OnkarRuikar]

including the conflicting changes from https://github.com/mdn/yari/pull/6352,

It introduced wrong hash for the current script on the main page (index.html). I've created a PR to fix it: https://github.com/mdn/yari/pull/7050.

[caugner]

It introduced wrong hash for the current script on the main page (index.html). I've created a PR to fix it: #7050.

Thank you for that. Otherwise this PR looks good to you?

[OnkarRuikar]

It introduced wrong hash for the current script on the main page (index.html). I've created a PR to fix it: #7050.

Thank you for that. Otherwise this PR looks good to you?

Yes, it works as expected. We can merge this.

---

I've committed a small improvement to the usePersistFrequentlyViewed function. It avoids figuring out a new index when a page is revisited.

[caugner]

Thank you @OnkarRuikar! 🎉

PS: I had second thoughts about my proposed naming of that identifier, so I renamed it to serial.