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 storage config loading state not updated #7638

Merged
merged 4 commits into from
Apr 9, 2024

Conversation

eladlachmi
Copy link
Contributor

@eladlachmi eladlachmi commented Apr 8, 2024

Closes #7249

Change Description

Background

Please refer to the linked issue for a full description of the bug.

Root Cause and Solution

There are actually two issues here, which manifest in the same way:

1. loading property not updated

The object viewer initially renders in a loading state. The useStorageConfig context hook re-renders the component when the storage config is loaded (from server or memory). During it's render, the component checks config.loading. In the initial state of the context, loading is set to true. However, when setting the state received from the server, the loading property isn't set, so the component re-renders into the same loading state.

To fix this issue we spread the server response object and add a loading property with a value false. Once config.loading is set to false, the object viewer component re-renders correctly.

2. The useEffect hook in the storage config context was missing a dep on the logged in user

When there is no logged in user, fetchStorageConfig rejects with HTTP status 401. Since there was a missing dependency, the useEffect hook would never trigger a re-render of the context value and thus not trigger a re-render of components that use it. Adding the user from the useUser hook to the dependencies array triggers the chain of re-renders as expected.

@eladlachmi eladlachmi self-assigned this Apr 8, 2024
@eladlachmi eladlachmi added include-changelog PR description should be included in next release changelog area/UI Improvements or additions to UI labels Apr 8, 2024
@eladlachmi eladlachmi requested a review from a team April 8, 2024 10:23
Copy link

github-actions bot commented Apr 8, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

13 passed

Copy link
Member

@N-o-Z N-o-Z left a comment

Choose a reason for hiding this comment

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

Thanks! Was waiting for this one :)

@itaiad200
Copy link
Contributor

@eladlachmi my understanding of the root cause is different, let me know what I missed. AFAIU when the user isn't logged in the storage-provider is still being called and returns 401, hence not setting it. Once the user is logged in, the storage-provider isn't being called again therefore it is stuck in a loading state. I don't see how this change fixes the problem during logins.

@eladlachmi
Copy link
Contributor Author

@itaiad200 You're right. Those are two separate issues that manifest with the same symptoms. On it...

@eladlachmi eladlachmi requested a review from itaiad200 April 8, 2024 12:18
@itaiad200
Copy link
Contributor

@itaiad200 You're right. Those are two separate issues that manifest with the same symptoms. On it...

The issue has 2 root causes? That's rare. How was this tested? I believe the issue isn't resolved by these fixes

@eladlachmi
Copy link
Contributor Author

The issue has 2 root causes?

@itaiad200
No. Two different issues end up with the same final result, i.e., they render the same HTML output.
Case no. 1: config has values, but loading is undefined after fetch. The component re-renders back into the loading state.
Case no. 2: config doesn't have values, because it doesn't re-fetch after login. The component doesn't re-render at all, so it remains in the loading state.

How was this tested?

This was tested manually and by the E2E tests that are part of this PR.

I believe the issue isn't resolved by these fixes

Maybe... How was this belief tested?

@itaiad200
Copy link
Contributor

The issue has 2 root causes?

@itaiad200 No. Two different issues end up with the same final result, i.e., they render the same HTML output. Case no. 1: config has values, but loading is undefined after fetch. The component re-renders back into the loading state. Case no. 2: config doesn't have values, because it doesn't re-fetch after login. The component doesn't re-render at all, so it remains in the loading state.

How was this tested?

This was tested manually and by the E2E tests that are part of this PR.

I believe the issue isn't resolved by these fixes

Maybe... How was this belief tested?

I experimented with this issue before. I alo checked out this branch and was able to reproduce the issue. I might have configured something wrong, but from my previous tries, the StorageProvider needs to be called again after a successful login. I don't know React well enough to understand the deps/context handling and if it happens in this fix.

@eladlachmi
Copy link
Contributor Author

the StorageProvider needs to be called again after a successful login. I don't know React well enough to understand the deps/context handling and if it happens in this fix.

Yeah, that's exactly what this change does

Copy link
Contributor

@itaiad200 itaiad200 left a comment

Choose a reason for hiding this comment

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

Thank you for your patience throughout this PR.

@eladlachmi eladlachmi merged commit 1bdf3b1 into master Apr 9, 2024
36 checks passed
@eladlachmi eladlachmi deleted the 7249-storage-config-loading-state-not-updated branch April 9, 2024 09:15
emulatorchen pushed a commit to emulatorchen/lakeFS that referenced this pull request May 27, 2024
* Fix storage config loading state not updated
* E2E test coverage for fix
* useEffect hook missing dep on logged in user
* E2E test coverage for logout/login scenario
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/UI Improvements or additions to UI include-changelog PR description should be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lakes.parquet file not opening in 1.6.0 in the Quickstart tutorial
3 participants