paranext / paranext-core

Electron client, extension host, and C# library for Paranext
https://paranext.github.io/paranext-core/
MIT License
17 stars 2 forks source link

Fix race condition with `isLoading` in `use-data.hook.ts` #597

Open lyonsil opened 1 year ago

lyonsil commented 1 year ago

Describe the bug When changing BCV repeatedly, the resource viewer will occasionally get stuck showing Loading instead of the right part of scripture. It appears to be related to the isLoading flag in this part of the resource-viewer.web-view.tsx:

  return (
    <div>
      {isLoading ? (
        'Loading'
      ) : (
        <ScriptureTextPanelUsxEditor usx={usx ?? '<usx/>'} onChanged={setUsx} />
      )}
    </div>
  );

When the code is changed to ignore isLoading, the resource viewer continues to show scripture properly as expected.

  return (
    <div>
        <ScriptureTextPanelUsxEditor usx={usx ?? '<usx/>'} onChanged={setUsx} />
    </div>
  );

To Reproduce Steps to reproduce the behavior:

  1. Make sure the resource viewer control is using the isLoading flag from use-data.hook.ts.
  2. Open Platform.Bible with the resource viewer open.
  3. Repeatedly using the < and/or > button to change books and chapters in quick succession.
  4. Eventually the USX viewer will get stuck on Loading. Changing BCV again usually clears it up after one or two updates, but sometimes it takes more.

Expected behavior The resource viewer should always update to scripture that matches the BCV control.

This is likely an issue with the pattern that people are expected to use isLoading from use-data.hook.ts, not something specifically wrong with the resource viewer per se. Either there is a bug in use-data.hook.ts or the documentation is suggesting the use of return values in a way that is not thread safe across async calls.

tjcouch-sil commented 1 year ago

I guess it has to do with how we're running setIsLoading in the hook in two different async functions. Just feels a bit shifty.

lyonsil commented 1 year ago

I saw this in the Text Collection extension today, too. Note the "Loading" text beside HPB despite the fact that HPB was loading fine since you could see the whole chapter to the right of it in the same webview. Unfortunately just changing the BCV doesn't clear it (at least not in this control).

Image