[WEB-55] Daily view site change events#1965
Conversation
Thread the clinician's site-change source into the Daily chart and gate the Events-Shown label on site-change presence: - patientdata: resolve the source via getSiteChangeSource(patient, manufacturer) so the Daily view defaults it per-manufacturer, matching the Basics view (no manual selection required). - daily: thread siteChangeSource through <Daily> into the tideline factory options; compute hasSiteChangeEventsInView from in-view tags.siteChange data alongside the existing alarm flag. - eventsInfoLabel: show the info tooltip when alarm OR site-change events are in view. - Export the inner DailyChart class so its chart options are unit testable. Add unit coverage for the threading, the in-view flag, and the widened label condition.
There was a problem hiding this comment.
Pull request overview
This PR updates the Daily view to support “site change” events by threading a new siteChangeSource option into the tideline daily chart and extending the Events label tooltip behavior to appear when site-change events are in view (not only alarm events). It also bumps the @tidepool/viz and tideline dependencies to versions that support the new functionality, and adds unit tests covering the updated behavior.
Changes:
- Bump
@tidepool/vizandtidelinedependency versions for WEB-55 support. - Compute and pass
siteChangeSourcefromPatientData→Daily→ tideline daily chart options. - Show the Events info tooltip when alarms or site-change events are within the current endpoints range; add unit test coverage for both the label and Daily behavior.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Updates lockfile to the new @tidepool/viz and tideline versions required for site change events support. |
| package.json | Bumps app version and updates direct dependency versions for @tidepool/viz and tideline. |
| app/pages/patientdata/patientdata.js | Computes and passes siteChangeSource into the Daily view component. |
| app/components/chart/eventsInfoLabel.js | Extends tooltip display condition to include site-change events-in-view. |
| app/components/chart/daily.js | Threads siteChangeSource to tideline chart options; computes hasSiteChangeEventsInView and passes it to EventsInfoLabel. |
| test/unit/components/chart/eventsInfoLabel.test.js | Adds unit tests validating tooltip visibility rules for the Events label. |
| test/unit/components/chart/daily.test.js | Adds unit tests for site-change tooltip behavior, siteChangeSource threading, and hasSiteChangeEventsInView computation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const hasAlarmEventsInView = _.some( | ||
| _.filter(nextProps.data.data.combined, d => !!d.tags?.alarm), | ||
| d => d.normalTime >= nextProps.data.data.current.endpoints.range[0] && d.normalTime <= nextProps.data.data.current.endpoints.range[1] | ||
| isInView | ||
| ); | ||
|
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughDaily charts now propagate site-change source metadata and track site-change events within visible endpoints. The events tooltip renders for either alarm or site-change events, with tests covering propagation, visibility, and tooltip behavior. ChangesDaily site-change event handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PatientData
participant Daily
participant DailyChart
participant EventsInfoLabel
participant ChartEndpoints
PatientData->>Daily: pass siteChangeSource
Daily->>DailyChart: pass translated chart props
Daily->>ChartEndpoints: evaluate alarm and site-change events
ChartEndpoints-->>Daily: return visibility flags
Daily->>EventsInfoLabel: pass alarm and site-change visibility
EventsInfoLabel-->>Daily: render tooltip when either flag is true
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
app/components/chart/daily.jsParsing error: [BABEL] /app/components/chart/daily.js: Using app/components/chart/eventsInfoLabel.jsParsing error: [BABEL] /app/components/chart/eventsInfoLabel.js: Using app/pages/patientdata/patientdata.jsParsing error: [BABEL] /app/pages/patientdata/patientdata.js: Using
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/unit/components/chart/daily.test.js (1)
317-317: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
constfor immutable fixture props.
dayDataReadyPropsis not reassigned.Proposed fix
- var dayDataReadyProps = _.assign({}, baseProps, { + const dayDataReadyProps = _.assign({}, baseProps, {As per coding guidelines, “Use ES6 features:
const/letinstead ofvar.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/components/chart/daily.test.js` at line 317, Change the declaration of the immutable fixture object dayDataReadyProps from var to const, leaving its initialization and subsequent usage unchanged.Source: Coding guidelines
app/components/chart/eventsInfoLabel.js (1)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare PropTypes for both visibility props.
Add
hasAlarmEventsInViewandhasSiteChangeEventsInViewas boolean PropTypes onEventsInfoLabel. As per coding guidelines, “Define PropTypes for all component props.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/chart/eventsInfoLabel.js` at line 13, Add boolean PropTypes for hasAlarmEventsInView and hasSiteChangeEventsInView on the EventsInfoLabel component, alongside its existing prop definitions, so both destructured visibility props are explicitly declared.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/components/chart/eventsInfoLabel.js`:
- Line 13: Add boolean PropTypes for hasAlarmEventsInView and
hasSiteChangeEventsInView on the EventsInfoLabel component, alongside its
existing prop definitions, so both destructured visibility props are explicitly
declared.
In `@test/unit/components/chart/daily.test.js`:
- Line 317: Change the declaration of the immutable fixture object
dayDataReadyProps from var to const, leaving its initialization and subsequent
usage unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b0ea3836-e9a5-49ce-b568-9e726772c9dc
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
app/components/chart/daily.jsapp/components/chart/eventsInfoLabel.jsapp/pages/patientdata/patientdata.jspackage.jsontest/unit/components/chart/daily.test.jstest/unit/components/chart/eventsInfoLabel.test.js
WEB-55
Related PRs:
tidepool-org/viz#651
tidepool-org/tideline#553