Conversation
- Enhanced the custom report generation to include trend charts based on user selections. - Introduced new endpoints for fetching available trend data and generating trend charts. - Updated the CustomReportRequest schema to support trend chart selections. - Modified the PDF generator to include a section for trend charts. - Improved the user interface to allow selection of trend charts alongside medical records. - Added localization support for trend chart features in multiple languages. - Updated relevant hooks and components to manage trend chart state and actions.
- Replaced time range selections with date range inputs for vital sign and lab test trend charts in custom reports. - Updated schemas to support date_from and date_to fields instead of time_range. - Modified relevant API endpoints and services to handle date range filtering. - Enhanced the user interface to reflect changes in date selection for trend charts. - Updated localization files to remove time range options and include date range labels. - Adjusted tests to validate new date range functionality in trend chart requests.
- Updated the PDF generator to include a "Count" column in the systolic and diastolic statistics tables. - Modified the trend chart generator to remove unnecessary statistics text and streamline data point representation. - Improved date tick handling in the calendar function for better visualization of data ranges.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds end-to-end support for including vital sign and lab test trend charts in custom medical reports (API → service layer → PDF rendering → frontend report builder UI), enabling reports to be generated with records, charts, or both.
Changes:
- Introduces trend chart request/selection schemas and updates custom report schemas to accept optional
trend_chartswith “must include something” validation. - Adds backend services for fetching trend data, generating PNG charts, and rendering a new “Trend Charts” section in the report PDF; exposes new API endpoints for available trend data and per-chart counts.
- Updates the frontend report builder to switch between record selection and trend chart selection, including chart count badges and localized strings.
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_trend_chart_schemas.py | Adds schema validation tests for trend chart request models. |
| tests/test_trend_chart_generator.py | Adds tests for PNG chart generation behavior. |
| tests/test_custom_report_with_charts.py | Tests custom report schema behavior with records-only, charts-only, and mixed requests. |
| requirements.txt | Adds matplotlib dependency for server-side chart rendering. |
| frontend/src/types/trendCharts.ts | Adds TS types matching backend trend chart schemas. |
| frontend/src/services/api/index.js | Adds API client methods to fetch available trend data and chart counts. |
| frontend/src/pages/reports/ReportBuilder.jsx | Adds segmented UI for switching between Records and Trend Charts; updates generate button labeling. |
| frontend/src/hooks/useCustomReports.js | Adds trend chart state/actions; includes trend_charts in report generation payload and validation. |
| frontend/src/components/reports/index.js | Exports the new TrendChartSelector component. |
| frontend/src/components/reports/TrendChartSelector.tsx | Implements trend chart selection UI with date range inputs and per-chart counts. |
| frontend/src/components/reports/TrendChartSelector.test.tsx | Adds component tests for trend chart selection UI interactions. |
| frontend/public/locales/*/common.json | Adds i18n strings for chart selection UI and new generate button labels. |
| app/services/trend_data_fetcher.py | Implements DB-backed fetching/counting of trend data + statistics preparation for charts. |
| app/services/trend_chart_generator.py | Generates print-friendly PNG charts using matplotlib (OO API). |
| app/services/custom_report_service.py | Enables report generation with records and/or charts; integrates chart generation into PDF payload. |
| app/services/custom_report_pdf_generator.py | Renders a new “Trend Charts” section in the PDF including chart images and stats tables. |
| app/schemas/trend_charts.py | Adds Pydantic models/validation for chart selection and date ranges. |
| app/schemas/custom_reports.py | Extends request/template schemas to include optional trend_charts and adds model-level “must include content” validation. |
| app/api/v1/endpoints/lab_test_component.py | Refactors lab trend direction computation to use regression-based helper. |
| app/api/v1/endpoints/custom_reports.py | Adds /available-trend-data and /trend-chart-counts endpoints; relaxes validation to allow charts-only reports. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/services/trend_data_fetcher.py
Outdated
| components = crud_lab_test_component.get_by_patient_and_test_name( | ||
| self.db, | ||
| patient_id=patient_id, | ||
| test_name=test_name, | ||
| date_from=date_from, | ||
| date_to=date_to, | ||
| ) | ||
| return len(components) | ||
|
|
||
|
|
There was a problem hiding this comment.
count_lab_test_records loads all matching LabTestComponent rows via get_by_patient_and_test_name(...) just to compute a count. For patients with lots of results this is unnecessarily expensive and will slow down /trend-chart-counts (which can be called frequently from the UI). Prefer a SQL COUNT(*) query that reuses the same filters as get_by_patient_and_test_name (or add a dedicated CRUD count method).
| components = crud_lab_test_component.get_by_patient_and_test_name( | |
| self.db, | |
| patient_id=patient_id, | |
| test_name=test_name, | |
| date_from=date_from, | |
| date_to=date_to, | |
| ) | |
| return len(components) | |
| # Use a direct COUNT(*) query instead of loading all components into memory. | |
| # We mirror the name handling used in get_available_lab_test_names. | |
| name_expr = func.coalesce( | |
| LabTestComponent.canonical_test_name, | |
| LabTestComponent.test_name, | |
| ) | |
| query = ( | |
| self.db.query(func.count(LabTestComponent.id)) | |
| .join(LabTestComponent.lab_result) | |
| .filter( | |
| LabResult.patient_id == patient_id, | |
| LabTestComponent.value.isnot(None), | |
| name_expr == test_name, | |
| ) | |
| ) | |
| # Note: date_from and date_to are currently unused here because the relevant | |
| # date field is not visible in this module. If needed, the same date filters | |
| # as in the underlying CRUD method can be added once that implementation is known. | |
| return query.scalar() or 0 |
| @router.post("/trend-chart-counts") | ||
| async def get_trend_chart_counts( | ||
| chart_selection: TrendChartSelection, | ||
| request: Request, | ||
| current_user_id: int = Depends(get_current_user_id), | ||
| db: Session = Depends(get_db) | ||
| ): | ||
| """ | ||
| Get record counts for selected trend charts filtered by their time ranges. | ||
| Used by the UI to show how many data points each chart will include. | ||
| """ | ||
| try: | ||
| patient_id = _get_active_patient_id(db, current_user_id) | ||
| if not patient_id: | ||
| return {"vital_counts": {}, "lab_test_counts": {}} | ||
|
|
||
| fetcher = TrendDataFetcher(db) | ||
|
|
||
| vital_counts = { | ||
| chart.vital_type: fetcher.count_vital_records( | ||
| patient_id, chart.vital_type, chart.date_from, chart.date_to, | ||
| ) | ||
| for chart in chart_selection.vital_charts | ||
| } | ||
|
|
||
| lab_test_counts = { | ||
| chart.test_name: fetcher.count_lab_test_records( | ||
| patient_id, chart.test_name, chart.date_from, chart.date_to, | ||
| ) | ||
| for chart in chart_selection.lab_test_charts | ||
| } | ||
|
|
||
| return { | ||
| "vital_counts": vital_counts, | ||
| "lab_test_counts": lab_test_counts, | ||
| } |
There was a problem hiding this comment.
This endpoint returns counts but does not log access/success via log_endpoint_access, unlike the other custom report endpoints in this module. Adding a success log (including chart counts and patient_id presence) would improve observability and make it easier to troubleshoot UI issues around trend chart selection.
tests/test_trend_chart_generator.py
Outdated
| @@ -0,0 +1,178 @@ | |||
| """Tests for trend chart generator (app/services/trend_chart_generator.py)""" | |||
|
|
|||
| import struct | |||
There was a problem hiding this comment.
struct is imported but never used in this test module. Removing the unused import will keep the test file clean and avoid failing stricter lint configurations.
| import struct |
| <DatePickerInput | ||
| value={chart.date_from} | ||
| onChange={(val) => updateVitalChartDates(chart.vital_type, val, chart.date_to)} | ||
| size="xs" | ||
| style={{ width: 130 }} | ||
| placeholder={t('reportBuilder.trendCharts.dateFrom')} | ||
| aria-label={t('reportBuilder.trendCharts.dateFrom')} | ||
| clearable | ||
| maxDate={chart.date_to || undefined} | ||
| popoverProps={{ withinPortal: true }} | ||
| /> | ||
| <DatePickerInput | ||
| value={chart.date_to} | ||
| onChange={(val) => updateVitalChartDates(chart.vital_type, chart.date_from, val)} | ||
| size="xs" | ||
| style={{ width: 130 }} | ||
| placeholder={t('reportBuilder.trendCharts.dateTo')} | ||
| aria-label={t('reportBuilder.trendCharts.dateTo')} | ||
| clearable | ||
| minDate={chart.date_from || undefined} | ||
| maxDate={today} | ||
| popoverProps={{ withinPortal: true }} |
There was a problem hiding this comment.
DatePickerInput expects Date | null values and minDate/maxDate as Date, but this component passes ISO date strings (e.g., chart.date_from / today) and forwards Date objects from onChange into update*Dates callbacks typed as string | null. This will cause type/runtime issues and can also send unexpected date formats to the backend. Convert between string (state/API) and Date (picker) at the boundary: parse strings to Date for the picker props, and serialize Date back to YYYY-MM-DD strings in the onChange handlers (and use new Date() for maxDate).
| # Get the active patient information | ||
| user = self.db.query(User).filter(User.id == user_id).first() | ||
| patient = self.db.query(Patient).filter(Patient.id == user.active_patient_id).first() | ||
|
|
||
|
|
||
| if not patient: | ||
| raise CustomReportError("No active patient found") | ||
|
|
There was a problem hiding this comment.
When request.selected_records is empty (charts-only report), validate_record_ownership is skipped, but this method still assumes user exists (user.active_patient_id). If the user record is missing this will raise an AttributeError and return a 500 instead of a controlled error. Add an explicit if not user: ... guard (and ideally validate active_patient_id) even in the charts-only path.
| # Trend direction using linear regression slope | ||
| from app.services.trend_data_fetcher import _compute_trend_direction | ||
| trend = _compute_trend_direction(values) |
There was a problem hiding this comment.
This endpoint imports and calls _compute_trend_direction from TrendDataFetcher, but the leading underscore indicates it's a private helper. Exposing this logic via a public utility (or moving it into a shared stats module) will avoid relying on private internals and makes future refactors safer.
| const getDefaultDateFrom = () => { | ||
| const d = new Date(); | ||
| d.setFullYear(d.getFullYear() - 1); | ||
| return d.toISOString().slice(0, 10); | ||
| }; | ||
|
|
||
| const getDefaultDateTo = () => new Date().toISOString().slice(0, 10); |
There was a problem hiding this comment.
Using toISOString().slice(0, 10) to build default date strings can produce off-by-one-day results depending on the user's timezone (because toISOString() is UTC). Prefer generating YYYY-MM-DD from local time (or store Date objects and format on API submit) so the default date range matches what the user expects.
| # Generate trend charts if requested | ||
| trend_chart_data = [] | ||
| if request.trend_charts: | ||
| trend_chart_data = self._generate_trend_charts( | ||
| patient.id, request.trend_charts | ||
| ) |
There was a problem hiding this comment.
Trend chart generation (matplotlib + data fetches) runs synchronously inside an async request handler. This can block the event loop and reduce API throughput when reports include charts. Consider running _generate_trend_charts in a threadpool (e.g., starlette.concurrency.run_in_threadpool) or making it async and offloading CPU-bound rendering, so chart-heavy reports don't stall other requests.
| # Inline import to avoid circular dependency with lab_test_component endpoint | ||
| from app.api.v1.endpoints.lab_test_component import calculate_trend_statistics | ||
| statistics = calculate_trend_statistics(components) | ||
|
|
There was a problem hiding this comment.
fetch_lab_test_trend imports calculate_trend_statistics from an API endpoint module. This creates a service->endpoint dependency and increases the risk of circular imports (and makes the statistics logic harder to reuse/test independently). Move the shared statistics function into a service/util module (e.g., app/services/lab_test_statistics.py) and import it from both the endpoint and TrendDataFetcher.
- Added logging for trend chart counts retrieval in the custom reports endpoint to improve traceability. - Refactored trend direction computation to utilize a utility function for better code organization. - Updated date handling in the frontend to format dates consistently for trend chart queries. - Improved error handling in the custom report service to raise an error when a user is not found. - Streamlined lab test component querying logic to enhance performance and maintainability.
This pull request adds support for including trend charts (vital signs and lab tests) in custom medical reports. It introduces new API endpoints and schema models for trend chart selection, updates the PDF generation logic to render charts and their statistics, and improves validation for report content. The changes also refactor how record selection is handled, allowing reports to be generated with either selected records, trend charts, or both.
Key changes include:
Trend Chart Support in Custom Reports
app/schemas/trend_charts.pyto represent trend chart requests, including validation for supported vital types, lab test names, and chart limits. ([app/schemas/trend_charts.pyR1-R91](https://github.com/afairgiant/MediKeep/pull/613/files#diff-85065804e41d38e2378af898bfdbeb882f26c8e663600b432cb6d31208d8ca8dR1-R91))CustomReportRequestandReportTemplateschemas to accept an optionaltrend_chartsfield, and added a model-level validator to require at least one of records or trend charts in a report. ([[1]](https://github.com/afairgiant/MediKeep/pull/613/files#diff-f9987cca62f1c096ae802a471192295829247dfd7a98deefe98651ce8e6ba27cL46-R54),[[2]](https://github.com/afairgiant/MediKeep/pull/613/files#diff-f9987cca62f1c096ae802a471192295829247dfd7a98deefe98651ce8e6ba27cR95-R115))API Endpoints for Trend Chart Data
app/api/v1/endpoints/custom_reports.py:/available-trend-data(GET) to list available vital types and lab tests for charting./trend-chart-counts(POST) to return record counts for selected charts and date ranges. ([app/api/v1/endpoints/custom_reports.pyR141-R236](https://github.com/afairgiant/MediKeep/pull/613/files#diff-9a0754ff4b89855952af277e3bf01431235c8c54e916f964c84657474f7ed99bR141-R236))[[1]](https://github.com/afairgiant/MediKeep/pull/613/files#diff-9a0754ff4b89855952af277e3bf01431235c8c54e916f964c84657474f7ed99bL77-R88),[[2]](https://github.com/afairgiant/MediKeep/pull/613/files#diff-9a0754ff4b89855952af277e3bf01431235c8c54e916f964c84657474f7ed99bL8-R37))PDF Generation Enhancements
[[1]](https://github.com/afairgiant/MediKeep/pull/613/files#diff-25e2993449af9ffef1de462115c0b592b6a83434e9144237d942875411af2539L360-R366),[[2]](https://github.com/afairgiant/MediKeep/pull/613/files#diff-25e2993449af9ffef1de462115c0b592b6a83434e9144237d942875411af2539R2043-R2183))Service Layer and Validation Improvements
[[1]](https://github.com/afairgiant/MediKeep/pull/613/files#diff-8643d69a60b4e2fa10c162fb3a70414bc61d114b53cd2ab4b12fd6b88dd43e3aL816-R833),[[2]](https://github.com/afairgiant/MediKeep/pull/613/files#diff-8643d69a60b4e2fa10c162fb3a70414bc61d114b53cd2ab4b12fd6b88dd43e3aL849-R876))[app/api/v1/endpoints/lab_test_component.pyL549-R551](https://github.com/afairgiant/MediKeep/pull/613/files#diff-ac6ec892ca16d4257cf4c6f475f62c51623800c358f31207096f396f90067172L549-R551))These changes collectively enable flexible, user-driven custom reports that can include both tabular data and visual trend charts, improving the utility and presentation of exported medical reports.