fix: Stop series data not saving when max values reached [PT-187949831]

[dougmartin]

Previously there was a useCallback() wrapper on the stop time series function that depended on the formData but this caused issues with the formData being captured as an empty array at the start of multiple runs where the runs hit their max number of values.

This fix adds a ref to the formData so that the closure is not created.

[pjanik]

Well, I somehow can't connect this explanation with the code changes. The previous useCallback() looked perfectly fine to me. The onSensorStopTimeSeries handler was recreated with each formData update, as it probably should be.

I could imagine that some code was calling setFormData incorrectly, most likely without creating a new object, and hence your callback was not updated. I think this could be a great opportunity to track it down, actually.

The current approach looks a bit brutal. I'm a bit worried we might be dealing with the issue at a deeper level, and it might surface later, making it harder to debug.

I feel that simply removing useCallback, even without using a ref, could have a similar result to what we currently have, wouldn't it? Although that still probably doesn't solve the core issue.

But I might not understand it well enough.

[dougmartin]

This function:

stopTimeSeriesFnRef.current = sensor.collectTimeSeries(timeSeriesCapabilities.measurementPeriod, selectableSensorId, (values) => {
   ...
}

closed over the current value of formData in onSensorStopTimeSeries even though there is a useCallback that updates onSensorStopTimeSeries when formData changes.

I've tested this change and it works for both the manual stop button (which worked before) and the automatic stop (which was not working).

[pjanik]

Ah, I see it better now. However, I think we're fixing it on the wrong side, and there are definitely more issues in this code. If we add anything else (e.g., a new state) to the onSensorStopTimeSeries, the problem happens again, and it won't be fun to debug.

I think the last argument of stopTimeSeriesFnRef is essentially wrong and this part needs to be fixed. I think it should either:

1. Not reference any state, just refs, or use functional updates of state
2. Probably be wrapped in useCallback with dependencies

Currently, it references formData that is surely outdated, and the same with rowIdx. It might work just fine for data collection, and it might not sound that important, but I think this is an important concept in general. If not here, then in other places, and it's probably good to be on the same page (also, if I'm wrong here).

Probably both fixes should be combined, and it might look like this:

    stopTimeSeriesFnRef.current = sensor.collectTimeSeries(timeSeriesCapabilities.measurementPeriod, selectableSensorId, useCallback((values) => {
        if (values.length <= MaxNumberOfTimeSeriesValues) {
          setFormData(prevData => {
            const newData = prevData.slice();
            newData[rowIdx] = {...newData[rowIdx], timeSeries: values, timeSeriesMetadata};
            return newData;
          });
        }
        if (values.length === MaxNumberOfTimeSeriesValues) {
          onSensorStopTimeSeries();
        }
      }, [rowIdx, onSensorStopTimeSeries, /* probably more things here */]);
      timeSeriesRecordingRowRef.current = rowIdx;
    };

And onSensorStopTimeSeries could use useCallback again.

[dougmartin]

@pjanik I tried your code and the callback passed to collectTimeSeries and invoked with each time series data collection still closes over the current onSensorStopTimeSeries function value and causes the bug to appear. This project is out of money and the current fix works so I think we should go with it.

[pjanik]

Yeap, I haven't tested it, but I'd think that's the correct direction.

But sure, let's merge it as it is.