Allow rendering locally stored viz data in patient data views#1846
Allow rendering locally stored viz data in patient data views#1846clintonium-119 wants to merge 5 commits intodevelopfrom
Conversation
Release 1.92.1 to master
There was a problem hiding this comment.
Pull request overview
This pull request adds support for rendering locally stored visualization data in patient data views. The implementation adds a localDataSource parameter that flows through the data fetching pipeline, from URL query parameters to the data worker, enabling developers to use local data files (e.g., local/rawData.json) for testing and development purposes.
Changes:
- Updated
@tidepool/vizandtidelinedependencies to versions with local data source support - Added
localDataSourceparameter throughout the data fetching flow (query extraction, action creators, worker) - Updated tests to verify the new parameter is properly passed
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Updated version and dependencies for @tidepool/viz and tideline to local-data-source versions |
| yarn.lock | Updated checksums for the new dependency versions |
| app/pages/patientdata/patientdata.js | Extract localDataSource from query parameters and pass to fetchers |
| app/redux/actions/async.js | Added localDataSource to fetchPatientData options with default value of false |
| app/redux/actions/worker.js | Added localDataSource parameter to dataWorkerAddDataRequest action creator |
| app/worker/DataWorker.js | Added conditional logic to call addLocalData when localDataSource is truthy |
| test/unit/redux/actions/async.test.js | Updated test assertions to include localDataSource: false |
| test/unit/redux/actions/worker.test.js | Updated test assertions to include localDataSource: undefined |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| /* Data Worker */ | ||
| export function dataWorkerAddDataRequest(data = [], returnData, patientId, fetchedUntil, oneMinCgmFetchedUntil ) { | ||
| export function dataWorkerAddDataRequest(data = [], returnData, patientId, fetchedUntil, oneMinCgmFetchedUntil, localDataSource ) { |
There was a problem hiding this comment.
There is a trailing space after the localDataSource parameter. This should be removed for consistency with the rest of the codebase.
| export function dataWorkerAddDataRequest(data = [], returnData, patientId, fetchedUntil, oneMinCgmFetchedUntil, localDataSource ) { | |
| export function dataWorkerAddDataRequest(data = [], returnData, patientId, fetchedUntil, oneMinCgmFetchedUntil, localDataSource) { |
| const data = JSON.parse(action.payload.data); | ||
| const { patientId, returnData } = action.payload; | ||
| const result = this.dataUtil.addData(data, patientId, returnData); | ||
| const { patientId, returnData, localDataSource } = action.payload; | ||
| const result = localDataSource ? this.dataUtil.addLocalData(patientId, returnData, localDataSource) : this.dataUtil.addData(data, patientId, returnData); |
There was a problem hiding this comment.
When localDataSource is truthy, the data variable (which is parsed from JSON on line 40) is not used. This results in unnecessary parsing overhead. Consider moving the JSON.parse call inside the conditional so it's only executed when needed, or alternatively, only parse the data when localDataSource is falsy.
| const dexcom = utils.getDexcom(ownProps.location); | ||
| const medtronic = utils.getMedtronic(ownProps.location); | ||
| const cbgFilter = utils.getCBGFilter(ownProps.location); | ||
| const localDataSource = ownProps.location.query.localDataSource; |
There was a problem hiding this comment.
The localDataSource parameter is extracted from the query string but not validated or type-checked. Query parameters are always strings, so if the parameter is present (even as an empty string like "localDataSource="), it will be truthy and trigger the local data path. Consider converting it to a boolean or checking for specific truthy values to avoid unintended behavior.
| const localDataSource = ownProps.location.query.localDataSource; | |
| const localDataSourceParam = ownProps.location.query.localDataSource; | |
| const localDataSource = localDataSourceParam === 'true' || | |
| localDataSourceParam === '1' || | |
| localDataSourceParam === 'yes'; |
Supports using the local/rawData.json that we use for viz dataUtil exporation withing Tidepool Web patient data views
Related PRs:
tidepool-org/tideline#521
tidepool-org/viz#584