Fix Stdout and File Section not working/refreshing #4029
Conversation
jzwang43
changed the title
frontend/src/components/worksheets/BundleDetail/BundleDetail.jsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Added a suggestion to use .finally instead. This is because we have to call setFetchingMetadata / setPendingFileSummaryFetches when each network promise either succeeds or fails, and .finally avoids duplication. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/finally
Can you also test this locally to ensure it does refresh? I don't think the PR as it is now would have worked -- because setFetchingMetadata(false) is currently not called on success of fetcherMetadata.
| return fetchFileSummary(uuid, '/').then(function(blob) { | ||
| setPendingFileSummaryFetches((f) => f - 1); | ||
| setFileContents(blob); | ||
| setStderr(null); | ||
| setStdout(null); |
There was a problem hiding this comment.
we should replace this with .finally(() => setPendingFileSummaryFetches((f) => f - 1)) (so this will be called whether the promise succeeds or fails)
| ); | ||
| } else { | ||
| stateUpdate[name] = null; | ||
| } | ||
| }); | ||
| Promise.all(fetchRequests).then((r) => { | ||
| setPendingFileSummaryFetches((f) => f - 1); | ||
| setFileContents(stateUpdate['fileContents']); |
There was a problem hiding this comment.
we should replace this with .finally(() => setPendingFileSummaryFetches((f) => f - 1)) (so this will be called whether the promise succeeds or fails)
| .then(function(blob) { | ||
| stateUpdate[name] = blob; | ||
| }) | ||
| .catch((e) => { |
There was a problem hiding this comment.
we should replace this with .finally(() => setPendingFileSummaryFetches((f) => f - 1)) (so this will be called whether the promise succeeds or fails)
setFetchingMetadata(false) is called in Thanks for the suggestion of using finally, I'll update that. |
ooh interesting, where in the code? Can't seem to locate it |
| @@ -152,6 +166,7 @@ const BundleDetail = ({ | |||
| revalidateOnMount: true, | |||
| refreshInterval: refreshInterval, | |||
| onSuccess: (response) => { | |||
| setFetchingMetadata(false); | |||
There was a problem hiding this comment.
@epicfaace it's here:)
There was a problem hiding this comment.
Wait, why do we call setFetchingMetadata after call fetcherContents? Shouldn't we call setFetchingMetadata only after calling fetcherMetadata?
There was a problem hiding this comment.
Yeah just cleaned up everything with finally
|
Great!
…--
Ashwin Ramaswami
On Wed, Mar 2, 2022 at 5:21 PM mergify[bot] ***@***.***> wrote:
Merged #4029 <#4029>
into master.
—
Reply to this email directly, view it on GitHub
<#4029 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAM4MX5YSUFJVLIC3RWDZR3U57SYDANCNFSM5PCWL3GA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Reasons for making this change
Previously the front end was constantly sending request to the back end even if the previous one was pending, added state variables to keep track of pending request so we don't overwhelm the back end.
Related issues
fixes #3994
Screenshots
No extra request when the call is pending.
Also tested
while sleep 2; do echo thinking; done, and it's updating file.Checklist