Heart Rate Chart #247

reneefromhold · 2024-04-24T21:19:54Z

Overview

This line chart will display the heart rate data from Fitbit Heart Rate Intraday, or AppleHealth via the DeviceDataV2 endpoints.

DataSources are AppleHealth and Fitbit
AggregateOptions are min, max, count, avg, sum
aggregationIntervalMinutes- the number of minutes to aggregate on
LineColor - the color of the line.
Thresholds - For each threshold defined, a reference line will be displayed and the color of the data line will render in the color defined.

Security

REMINDER: All file contents are public.

[x ] I have ensured no secure credentials or sensitive information remain in code, metadata, comments, etc.
- [ x] Please verify that you double checked that .storybook/preview.js does not contain your participant access key details.
- [ x] There are no temporary testing changes committed such as API base URLs, access tokens, print/log statements, etc.
[ x] These changes do not introduce any security risks, or any such risks have been properly mitigated.

Describe briefly what security risks you considered, why they don't apply, or how they've been mitigated.

Checklist

Testing

MyDataHelps iOS app
MyDataHelps Android app
[x ] MyDataHelps website

Documentation

I have added relevant Storybook updates to this PR as well.
If this feature requires a developer doc update, tag @CareEvolution/api-docs.

Consider "Squash and merge" as needed to keep the commit history reasonable on main.

Reviewers

Assign to the appropriate reviewer(s). Minimally, a second set of eyes is needed ensure no non-public information is published. Consider also including:

Subject-matter experts
Style/editing reviewers
Others requested by the content owner

src/components/container/IntradayHeartRateChart/IntradayHeartRateChart.tsx

chrisnowak · 2024-04-30T20:31:12Z

The x-axis here should be like the original mockup chart and just be spaced 3 hours apart with labels at 3am, 6am, 9am, 12pm, 3pm, 6pm, 9pm:

Additionally, I think a lot of the formatting / styling that I shared previously is not applied - e.g. this is showing solid black borders...my sense was that all we had to do was take the code from my branch and pipe in the data and fix up the gradient/threshold stuff

chrisnowak · 2024-04-30T20:38:23Z

src/components/presentational/LineChart/LineChart.tsx

+    innerRef?: React.Ref<HTMLDivElement>
+}
+
+export default function MdhLineChart(props: LineChartProps) {


I definitely wouldnt' call this "LineChart" as that is far too vague of a name. I think we should make this at least somewhat opininated.

Putting aside whether it makes sense to combine this with @ajmenca 's PR, I would propose that this be called "IntradayLineChart" and be hardcoded to a 24 hour interval with the x-axis as I described and it could take data as data points in that interval...do not think it makes sense to make a generally reusable "line chart" as that is what recharts already gives us

I do agree LineChart is too vague.
What I do want to avoid is recreating the threshold interface and the display of it across components. We currently consume BarChartThreshold in vbuilder. Adding LineChartThreshold here is no help. Would be nice to have a simple ChartThreshold interface that is used across charts, along with not having to copy/paste the display of thresholds across components.

I will integrate the feedback around making it more opinionated and hang tight on the Threshold stuff

chrisnowak · 2024-04-30T20:38:44Z

src/components/presentational/LineChart/LineChart.tsx

+        const xValues: any[] = data.map(d => d.date.getTime());
+        const xMax = Math.max(...xValues);
+        const xMin = Math.min(...xValues);
+        axis.xRange = [xMin, xMax];


Axis should be hardcoded

chrisnowak · 2024-04-30T20:39:06Z

src/components/presentational/LineChart/LineChart.tsx

+    if (!props.axis || (props.axis && !props.axis.yRange)) {
+        const yValues: number[] = data.map(d => d.value);
+        const yMax = Math.max(...yValues);
+        axis.yRange = [0, yMax];


Curious why you need to do this...I think just not supplying a domain will be sufficient

chrisnowak · 2024-04-30T20:39:47Z

src/components/presentational/LineChart/LineChart.tsx

+                <ReferenceLine key={`threshref_${index}`} y={threshold.value} stroke={resolveColor(layoutContext.colorScheme, threshold.referenceLineColor)} />
+            )}
+            <YAxis tick={YAxisTick} type='number' domain={axis?.yRange ?? [0,100]} />
+            <XAxis tick={XAxisTick} axisLine={true} dataKey="date" tickMargin={0} minTickGap={0} tickCount={24}


This is where I think you need to specify the "interval" to get the axis ticks to show up at 3/6/9 etc.

chrisnowak · 2024-04-30T20:42:01Z

src/components/container/IntradayHeartRateChart/IntradayHeartRateChart.tsx

+    previewState?: IntradayHeartRatePreviewState,
+    dataSource: DeviceDataV2Namespace,
+    aggregationOption: AggregationOption,
+    aggregationIntervalAmount: number,


Let's just call this "aggregationIntervalMinutes" and not have aggregationIntervalType...this is intraday so no need to complicate with extra params

chrisnowak · 2024-04-30T20:43:33Z

src/helpers/heart-rate-data-providers/intraday-aggregates.tsx

@@ -0,0 +1,29 @@
+import MyDataHelps, { Guid, DeviceDataV2AggregateQuery, DeviceDataV2AggregatePage, DeviceDataV2Aggregate } from "@careevolution/mydatahelps-js";
+
+export default async function (query: DeviceDataV2AggregateQuery): Promise<DeviceDataV2Aggregate[]> {


What does this have to do with heart rate data - this seems like a general "queryAllDeviceDataV2" function more than anything to do with heart rates?

So seems like it would go in the same bucket as query all survey answers etc.

chrisnowak · 2024-04-30T20:44:34Z

src/components/container/IntradayHeartRateChart/IntradayHeartRateChart.tsx

+
+export default function IntradayHeartRateChart(props: IntradayHeartRateChartProps) {
+    const [data, setData] = useState<LineChartDataPoint[]>([]);
+    const [axis, setAxis] = useState<LineChartAxis>({ yRange: [0, 200], yIncrement: 30, xIncrement: props.aggregationIntervalAmount });


I don't think yRange needs to be specified at all or yIncrement - in my branch it worked fine without having to specify these

The value of having some default x or y axis in place is so the chart renders when there is no data.
If nothing displays at all, the user may not be aware their device is is not connected. An empty chart could reinforce that the participant should connect their device

I think we would want to mimic the "Daily data chart"'s no data state here which does not display a y-axis but does display the x-axis

https://ui.mydatahelps.org/?path=/story/container-dailydatachart--no-data

chrisnowak · 2024-04-30T20:46:11Z

Something kind of strange - the line seems to fly in from the right as the animation? I think we'd want to use whatever animation the default DailyDataChart uses for instance (which I think kind of comes up from the bottom)...

chrisnowak · 2024-04-30T20:47:52Z

src/components/container/IntradayHeartRateChart/IntradayHeartRateChart.tsx

+
+export interface IntradayHeartRateChartProps {
+    previewState?: IntradayHeartRatePreviewState,
+    dataSource: DeviceDataV2Namespace,


Per the spec this should accept multiple, e.g. Apple Health AND Fitbit:

It should query data for all data sources specified as props; if there happens to be data for 2 data sources for an interval it should use the data source that is first in the array (e.g. Fitbit by default)

chrisnowak · 2024-04-30T20:51:29Z

src/components/container/IntradayHeartRateChart/IntradayHeartRateChart.tsx

+        dataPoints = dataPoints.sort((a, b) => a.date.getTime() - b.date.getTime());
+
+        if (dataPoints.length){
+            let yValues: number[] = dataPoints.map(d => d.value);


What happens if you have a gap in the middle of the day e.g. where the user wasn't wearing their device?

@chrisnowak The line will continue without breaks.

Right - my point is that is not the right behavior though, right? Since you need each heart rate data point to be represented at the time for the data point

This is corrected, and a story is in place for showing the behavior (line break).

chrisnowak · 2024-04-30T20:58:49Z

Broadly still a little confused...my branch seems like it was a good chunk of the way there on the presentation layer...seemed mostly like it just needed:

Querying the data from the aggregates
Converting the data into the proper format; probably doing some math on the axes compared to the aggregate interval
Fixing up the gradients
Cleaning up a bit

This seems to have gone off in a a bit of a different direction and doesn't quite match the spec or the example screenshot?

chrisnowak · 2024-04-30T20:59:36Z

Let's also figure out a way to get the line colors perfect - right now it obviously is a bit fuzzy - but the line should solidly change color when it is above the threshold...I can maybe help with that

…, generate all times for being able to showcase nulls as line gaps

reneefromhold · 2024-05-03T21:29:14Z

Something kind of strange - the line seems to fly in from the right as the animation? I think we'd want to use whatever animation the default DailyDataChart uses for instance (which I think kind of comes up from the bottom)...

This is now corrected when not using the date range navigator. When using the navigator the data will swing in from the same direction navigated to.

reneefromhold · 2024-05-03T21:30:13Z

Let's also figure out a way to get the line colors perfect - right now it obviously is a bit fuzzy - but the line should solidly change color when it is above the threshold...I can maybe help with that

@chrisnowak I've addressed most of the feedback, though getting the colors to sharply change at the threshold is still outstanding. Will work on that in next round of changes

reneefromhold · 2024-05-15T16:43:50Z

@chrisnowak The issue with line colors + incomplete data is fixed.

ClaysGit · 2024-05-15T17:30:06Z

@CodiumAI-Agent /review

CodiumAI-Agent · 2024-05-15T17:30:55Z

PR Review 🔍

⏱️ Estimated effort to review [1-5]	4, due to the complexity and size of the PR, which includes multiple new components and helper functions, integration with external APIs, and complex data handling and visualization logic.
🧪 Relevant tests	No
⚡ Possible issues	Possible Bug: The `getRandomInt` function in `IntradayHeartRateChart.data.ts` does not handle the case where `min` is greater than `max`, which could lead to incorrect behavior or runtime errors.
⚡ Possible issues	Data Integrity: The `loadForTest` function in `IntradayHeartRateChart.data.ts` assumes that all data entries have unique timestamps. If two data points have the same timestamp, one will overwrite the other.
🔒 Security concerns	No

Code feedback:

relevant file	src/components/container/IntradayHeartRateChart/IntradayHeartRateChart.data.ts
suggestion	Consider adding error handling for the case where `min` is greater than `max` in the `getRandomInt` function to prevent potential runtime errors. [important]
relevant line	function getRandomInt(min : number, max : number) {

relevant file	src/components/container/IntradayHeartRateChart/IntradayHeartRateChart.data.ts
suggestion	To ensure data integrity, modify the `loadForTest` function to handle cases where multiple data points have the same timestamp, perhaps by storing them in an array or using a more complex data structure. [important]
relevant line	function loadForTest(data: DeviceDataV2Aggregate[]) {

Start IntraDayHR Line chart

9b89549

reneefromhold requested a review from chrisnowak April 24, 2024 21:20

reneefromhold commented Apr 25, 2024

View reviewed changes

src/components/container/IntradayHeartRateChart/IntradayHeartRateChart.tsx Show resolved Hide resolved

Broke out line chart. Created index.ts left out of first pr

f0cd7f7

chrisnowak reviewed Apr 30, 2024

View reviewed changes

Refactoring to make line chart more opinionated, include apple health…

cd3b167

…, generate all times for being able to showcase nulls as line gaps

reneefromhold force-pushed the renee/HeartRate branch from a99d985 to cd3b167 Compare May 3, 2024 21:26

reneefromhold and others added 3 commits May 6, 2024 09:50

Cleaning up threshold transition

854d466

Perfected the stops on the linearGradient

828fee4

Merge branch 'main' into renee/HeartRate

c4d606a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Heart Rate Chart #247

Heart Rate Chart #247

reneefromhold commented Apr 24, 2024 •

edited

chrisnowak commented Apr 30, 2024

chrisnowak Apr 30, 2024

reneefromhold May 1, 2024

reneefromhold May 1, 2024

chrisnowak Apr 30, 2024

chrisnowak Apr 30, 2024

chrisnowak Apr 30, 2024

chrisnowak Apr 30, 2024

chrisnowak Apr 30, 2024

chrisnowak Apr 30, 2024

chrisnowak Apr 30, 2024

reneefromhold May 1, 2024

chrisnowak May 1, 2024

chrisnowak May 1, 2024

chrisnowak commented Apr 30, 2024

chrisnowak Apr 30, 2024

chrisnowak Apr 30, 2024

reneefromhold May 2, 2024

chrisnowak May 2, 2024

reneefromhold May 3, 2024

chrisnowak commented Apr 30, 2024

chrisnowak commented Apr 30, 2024

reneefromhold commented May 3, 2024

reneefromhold commented May 3, 2024

reneefromhold commented May 15, 2024

ClaysGit commented May 15, 2024

CodiumAI-Agent commented May 15, 2024

		@@ -0,0 +1,29 @@
		import MyDataHelps, { Guid, DeviceDataV2AggregateQuery, DeviceDataV2AggregatePage, DeviceDataV2Aggregate } from "@careevolution/mydatahelps-js";

		export default async function (query: DeviceDataV2AggregateQuery): Promise<DeviceDataV2Aggregate[]> {

Heart Rate Chart #247

Are you sure you want to change the base?

Heart Rate Chart #247

Conversation

reneefromhold commented Apr 24, 2024 • edited

Overview

Security

Checklist

Testing

Documentation

Reviewers

chrisnowak commented Apr 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisnowak commented Apr 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisnowak commented Apr 30, 2024

chrisnowak commented Apr 30, 2024

reneefromhold commented May 3, 2024

reneefromhold commented May 3, 2024

reneefromhold commented May 15, 2024

ClaysGit commented May 15, 2024

CodiumAI-Agent commented May 15, 2024

PR Review 🔍

reneefromhold commented Apr 24, 2024 •

edited