fix: [M3-8519] - Objects Table Refreshing Logic

dchyrva-akamai commented 1 month ago

Description 📝

After uploading several files into the object storage, file table was not updated correctly. In the provided fix, fresh data from the store is used during the store overriding.

Changes 🔄

Object buckets query key moved into a separate getter function and exported.
Fresh store data is used during the overriding process.

How to test 🧪

Open the "Object Storage" page.
Upload several files using drop area or browse button.
Look at how the table is being updated after each file is uploaded.

As an Author I have considered 🤔

Check all that apply

[x] 👀 Doing a self review
[x] ❔ Our contribution guidelines
[x] 🤏 Splitting feature into small PRs
[ ] ➕ Adding a changeset
[ ] 🧪 Providing/Improving test coverage
[ ] 🔐 Removing all sensitive information from the code and PR description
[ ] 🚩 Using a feature flag to protect the release
[x] 👣 Providing comprehensive reproduction steps
[ ] 📑 Providing or updating our documentation
[ ] 🕛 Scheduling a pair reviewing session
[ ] 📱 Providing mobile support
[ ] ♿ Providing accessibility support

github-actions[bot] commented 1 month ago

Coverage Report: ✅
Base Coverage: 86.64%
Current Coverage: 86.64%

dchyrva-akamai commented 1 month ago

Changes look good. Why we need to get the data using getQueryData rather than just using the data returned by the hook?

Hello @bnussman-akamai

When several objects are added simultaneously, the component is re-rendered only after all of them have been added. This means that the data from the hook is not updated with new values after each individual object is added, which causes a bug.

jaalah-akamai commented 1 month ago

Changes look good. Why we need to get the data using getQueryData rather than just using the data returned by the hook?

Hello @bnussman-akamai

When several objects are added simultaneously, the component is re-rendered only after all of them have been added. This means that the data from the hook is not updated with new values after each individual object is added, which causes a bug.

First of all, nice work - this works. I just want to toss out an alternative... I'm interested in what you all think about about reverting back to using the data from the hook (I like the term currentData so maybe just do data: currentData). Then invalidate the cache at the end of the maybeAddObjectToTable function:

if (bucket) {
      queryClient.invalidateQueries({
        queryKey: getObjectBucketObjectsQueryKey(
          clusterId,
          bucket.label,
          prefix
        ),
      });
    }

dchyrva-akamai commented 1 month ago

Changes look good. Why we need to get the data using getQueryData rather than just using the data returned by the hook?

Hello @bnussman-akamai When several objects are added simultaneously, the component is re-rendered only after all of them have been added. This means that the data from the hook is not updated with new values after each individual object is added, which causes a bug.

First of all, nice work - this works. I just want to toss out an alternative... I'm interested in what you all think about about reverting back to using the data from the hook (I like the term currentData so maybe just do data: currentData). Then invalidate the cache at the end of the maybeAddObjectToTable function:
if (bucket) {
      queryClient.invalidateQueries({
        queryKey: getObjectBucketObjectsQueryKey(
          clusterId,
          bucket.label,
          prefix
        ),
      });
    }

Hello @bnussman-akamai I tired it out, unfortunately it partially solves the issue, this approach will re-fetch the data each time new file is added which I believe is something we are trying to avoid with updateStore method, plus I think there will be too many requests to the back-end if we will fetch list of files each time a new file is added.

bnussman-akamai commented 1 month ago

I agree. We should keep updating the store manually.

When several objects are added simultaneously, the component is re-rendered only after all of them have been added.

I'm a bit surprised this is happening, but the fix makes sense!

linode / manager