Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Sep 24, 2024

Conversation

dchyrva-akamai
Copy link
Contributor

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

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 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
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@dchyrva-akamai dchyrva-akamai requested a review from a team as a code owner September 12, 2024 11:11
@dchyrva-akamai dchyrva-akamai requested review from jaalah-akamai and abailly-akamai and removed request for a team September 12, 2024 11:11
Copy link

github-actions bot commented Sep 12, 2024

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

@bnussman-akamai bnussman-akamai added the Object Storage deals with Object Storage label Sep 12, 2024
Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Also, we might be able to update the updateStore function to use the getObjectBucketObjectsQueryKey util you created

Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice concise fix @dchyrva-akamai 👍

Code looks good and confirmed fix with objects now showing up in the table underneath as they are uploaded. ✅

I also tested Obj feature as a whole (delete operations etc) and did not notice any regression ✅

@bnussman-akamai bnussman-akamai changed the title fix: [M3-8519] - Objects Table Refreshing Logic Fixed. fix: [M3-8519] - Objects Table Refreshing Logic Sep 13, 2024
@dchyrva-akamai
Copy link
Contributor Author

dchyrva-akamai commented Sep 16, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

dchyrva-akamai commented Sep 17, 2024

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
Copy link
Member

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!

Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making that getObjectBucketObjectsQueryKey helper function!

@jaalah-akamai jaalah-akamai added the Approved Multiple approvals and ready to merge! label Sep 17, 2024
@abailly-akamai abailly-akamai merged commit 4dd89b7 into linode:develop Sep 24, 2024
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Object Storage deals with Object Storage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants