[DC-875] Debounce Text Input and Slider

[evansuslovich]

Jira Ticket: https://broadworkbench.atlassian.net/browse/DC-875

Summary of changes:

What

• Debounce calling the backends for getting groupParticipantCount and criteriaCount.

Why

• Debouncing will wait for state to persist over an elapsed period of time before doing a certain action, in our situation, calling the backend for an individual item or a group of items.

Testing strategy

• Unit Testing

Before:

before-debounce-year-slider.mov

After:

debounce.text.input.and.slider.mov

[pshapiro4broad]

This looks OK, although it makes me wonder about the general problem we're solving here.

The reason we need to debounce is because eventually we end up calling a web API that's known to be slow. So another approach is to instead only wrap the web API itself in a debounce operation. If this approach works then we wouldn't have to add another debounce when a new UI element is added.

[evansuslovich]

This looks OK, although it makes me wonder about the general problem we're solving here.

The reason we need to debounce is because eventually we end up calling a web API that's known to be slow. So another approach is to instead only wrap the web API itself in a debounce operation. If this approach works then we wouldn't have to add another debounce when a new UI element is added.

So, debouncing DataRepo itself? Would this make every API endpoint debounced?

[rjohanek]

looks great!

[lstrano-broad]

Are we creating a problem for the user by not providing immediate visual feedback?

[pshapiro4broad]

So, debouncing DataRepo itself? Would this make every API endpoint debounced?

I was thinking the getCounts API to start with. If that works well, we can look at doing the same for searchConcepts(), although that has only one call site so it's less of an issue.

Also I think we'd want to provide both bounced and non-bounced versions of the API, in case a caller wants to compute the value right away. Otherwise any call would force the caller to wait for the bounce period (250ms) to expire.

I'm still not 100% sure that this approach will work.. it looks like there are some utilities that can be used to debounce a promise returning function, which the API calls are, so I'm hopeful that it might. For example https://www.npmjs.com/package/debounce-promise

[evansuslovich]

we want to use a useRef here because we only want one instance of the debounce function. If there are changes to any dependency inside of useRef (i.e datasetId, criteria) we can change it with .current()and passing in new values.

[pshapiro4broad]

This seems OK although I wonder if some refactoring and renaming could make the code clearer. Also, there is a third call to getCounts() in DatasetBuilder.ts. Doesn't that need a debounce as well?

Is there a way to unit test the debounce behavior so that we can be sure we don't break it in the future?

[pshapiro4broad]

Is this mock still necessary?

[evansuslovich]

I thought that since the tests were passing they were needed but I removed the mock and the tests are still passing.

[pshapiro4broad]

Would it be possible to split out the _.debounce(250, ...) into a separate method so it could be shared in both places? Also it seems like we'd want a way to mock it during tests, so our tests wouldn't have to wait for the debounce to timeout before an expected text appears in the UI.

[evansuslovich]

If we were to do this, we could get rid of the useRef because useRefs are only allowed to be used in a component. Then, we can create an abstract function to get the debounce out.

const debounceFunc = _.debounce(250, (func) => func);

Then, we would put the func inside of the useEffect like this:

  useEffect(() => {
    debounceFunc(
      setCriteriaCount(
        async () =>
          await DataRepo()
            .dataset(datasetId)
            .getCounts({
              cohorts: [
                {
                  // Create a "cohort" to get the count of participants for this criteria on its own.
                  criteriaGroups: [{ criteria: [criteria], name: '', meetAll: true, mustMeet: true }],
                  name: '',
                },
              ],
            })
      )
    );
  }, [criteria, datasetId, setCriteriaCount]);
  

There could certainly be a stronger function name for debounceFunc, but I personally think this will clutter the useEffect? We can't set this function:

      setCriteriaCount(
        async () =>
          await DataRepo()
            .dataset(datasetId)
            .getCounts({
              cohorts: [
                {
                  // Create a "cohort" to get the count of participants for this criteria on its own.
                  criteriaGroups: [{ criteria: [criteria], name: '', meetAll: true, mustMeet: true }],
                  name: '',
                },
              ],
            })
      )

as a variable outside of the useEffect because this is going to cause an infinite render error and we also can't put it inside of the useEffect because this will be undebounced.

[pshapiro4broad]

The name doesn't seem right to me. This is really debouncing the call to getCounts(); the fact that it also debounces setting the value once that API returns seems incidental, as debouncing the set operation has no effect (vs debouncing the API call, which does).

Maybe a better name is updateCriteriaCount which describes what this function is doing instead of how it's doing it?

[evansuslovich]

I agree that we aren't really debouncing setCriteriaCount but more calling debouncing getCounts(); and then setting it.

[evansuslovich]

This seems OK although I wonder if some refactoring and renaming could make the code clearer. Also, there is a third call to getCounts() in DatasetBuilder.ts. Doesn't that need a debounce as well?

Is there a way to unit test the debounce behavior so that we can be sure we don't break it in the future?

I don't think we have to debounce the getCounts()in DatasetBuilder.ts because the changes would be caused by the checkboxes in the 3 panels. I'll bring this up to discussion today.

[rjohanek]

This looks great thanks for the comments and videos!

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud