feat: Add basic chart for vital signs observations#19
Conversation
Add PRD-0004 defining: - Blood pressure and pulse vital signs recording - Time-series chart visualization with Chart.js - openEHR metadata transparency panel showing archetypes, paths, and raw compositions - API endpoints for vital signs CRUD and openEHR inspection - Template definition using blood_pressure.v2 and pulse.v2 archetypes
- Add backend support for recording and querying vital signs (blood pressure, pulse) - Create AQL queries for vital signs observations from EHRBase - Build FLAT format compositions for openEHR vital signs template - Add openEHR metadata endpoints for composition lookup and archetype info - Create VitalSignsChart component with Chart.js time-series visualization - Add RecordVitalsDialog for recording new vital signs - Add VitalSignsTable for tabular display of readings - Create OpenEHRMetadataPanel to show composition structure, archetypes, and path mappings - Update PatientDetailPage to include the vital signs panel - Configure ruff to allow FastAPI Query pattern Implements PRD-0004
WalkthroughAdds end-to-end vital-signs support with openEHR transparency: backend schemas, service, router, EHRBase client/templates/queries updates and startup template registration; frontend types, store and UI components (chart, table, record dialog, OpenEHR inspector); upload script and PRD/ADR docs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Web as Web UI
participant API as API Server
participant EHR as EHRBase
participant DB as EHR DB
Web->>API: POST /observations/vital-signs (VitalSignsCreate)
Note right of API `#E6F0FF`: validate payload\nbuild FLAT composition
API->>EHR: POST /ehr/{ehr_id}/composition (FLAT)
alt EHR success
EHR->>DB: persist composition
EHR-->>API: 201 + composition_uid
API->>API: build openehr_metadata
API-->>Web: 200 VitalSignsResponse (includes openehr_metadata)
else EHR failure
EHR-->>API: error (5xx/503)
API->>API: generate placeholder UID, log warning
API-->>Web: 200 VitalSignsResponse (placeholder metadata)
end
sequenceDiagram
autonumber
participant Web as Web UI
participant API as API Server
participant EHR as EHRBase
Web->>API: GET /observations/vital-signs?patient_id=P&from=X&to=Y
Note right of API `#F0FFF0`: execute AQL (VITAL_SIGNS_DATE_RANGE_QUERY)
API->>EHR: POST /query?aql
EHR-->>API: result set rows
API->>API: parse rows -> VitalSignsResponse[]
API-->>Web: VitalSignsListResponse (items, total)
Web->>Web: render chart & table (sorted by recorded_at)
sequenceDiagram
autonumber
participant Web as Web UI
participant API as API Server
participant EHR as EHRBase
Web->>API: GET /observations/vital-signs/{uid}/raw?format=FLAT
API->>EHR: GET /ehr/{ehr_id}/composition/{uid}?format=FLAT
EHR-->>API: composition payload
API-->>Web: RawCompositionResponse
Web->>Web: display in OpenEHRMetadataPanel (toggle format -> refetch)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)web/src/**/*.{ts,tsx,vue}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🔇 Additional comments (9)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
web/src/components/vitals/VitalSignsTable.vue (1)
22-37: Consider localization for date/time formatting.The
formatDateandformatTimefunctions hardcode'en-US'locale. For future i18n support, consider making this configurable or using the user's browser locale.docs/prd/0004-vital-signs-chart.md (1)
69-87: Add language specifier to fenced code blocks for linter compliance.Several fenced code blocks (lines 69, 231, 243, 354, 399, 436, 511, 520) lack language specifiers. While these are ASCII diagrams and endpoint listings, adding
textorplaintextwould satisfy the markdownlint rule MD040.🔎 Example fix
-``` +```text COMPOSITION (openEHR-EHR-COMPOSITION.encounter.v1) ├── contextweb/src/components/openehr/OpenEHRMetadataPanel.vue (2)
37-44: Consider parallel fetching for archetype info.The archetype info fetches are sequential. For better performance with multiple archetypes, consider using
Promise.all:🔎 Proposed refactor
- for (const archetypeId of reading.openehr_metadata.archetype_ids) { - if (!archetypeInfos.value.has(archetypeId)) { - const info = await store.fetchArchetypeInfo(archetypeId) - if (info) { - archetypeInfos.value.set(archetypeId, info) - } - } - } + const fetchPromises = reading.openehr_metadata.archetype_ids + .filter(id => !archetypeInfos.value.has(id)) + .map(async (archetypeId) => { + const info = await store.fetchArchetypeInfo(archetypeId) + if (info) { + archetypeInfos.value.set(archetypeId, info) + } + }) + await Promise.all(fetchPromises)
206-222: Tabs implementation mixes controlled and uncontrolled patterns.The
TabsRoothas:default-value="activeFormat"(uncontrolled), but theTabsTriggerelements have manual@clickhandlers callingswitchFormat. This works but is slightly redundant. Consider usingv-modelfor cleaner state management, or remove the click handlers and use the@update:modelValueevent onTabsRoot.web/src/components/vitals/VitalSignsPanel.vue (1)
59-67: Duplicate handler functions.
handleSelectReadingandhandleShowOpenEHRare identical. Consider consolidating into a single function.🔎 Proposed refactor
-function handleSelectReading(reading: VitalSignsReading) { - selectedReading.value = reading - showOpenEHRPanel.value = true -} - -function handleShowOpenEHR(reading: VitalSignsReading) { +function openOpenEHRPanel(reading: VitalSignsReading) { selectedReading.value = reading showOpenEHRPanel.value = true }Then use
openOpenEHRPanelfor both event handlers in the template.web/src/stores/vitals.ts (2)
61-78: Consider URL-encoding the patient_id query parameter.The
compositionUidis properly encoded, butpatientIdin the query string is not. While patient IDs are typically alphanumeric, it's safer to encode all user-provided values.🔎 Suggested fix
- await api.delete(`/api/observations/vital-signs/${encodeURIComponent(compositionUid)}?patient_id=${patientId}`) + await api.delete(`/api/observations/vital-signs/${encodeURIComponent(compositionUid)}?patient_id=${encodeURIComponent(patientId)}`)
80-120: Consider independent error state for metadata fetches.These helper functions set
error.valueon failure, which could overwrite errors from primary operations likefetchVitalsorrecordVitals. If a user is viewing an error from a failed vitals fetch, and then clicks to view metadata (which also fails), the original error context is lost.Consider either:
- Not setting the global error for these secondary operations
- Using a separate error state for metadata operations
- Logging errors without surfacing them in the shared state
This is a minor UX concern for edge cases.
api/src/observations/router.py (1)
110-136: Avoid accessing private_get_client()method directly in router.The router directly accesses
ehrbase_client._get_client()(line 118), which is a private method. This breaks encapsulation and couples the router to the client's internal implementation.Consider adding a public method to
ehrbase_clientfor fetching template examples, similar to howlist_templates()is already exposed.🔎 Suggested approach
Add a method to
api/src/ehrbase/client.py:async def get_template_example(self, template_id: str, format: str = "FLAT") -> dict[str, Any]: """Get template example in specified format.""" client = await self._get_client() response = await client.get( f"/openehr/v1/definition/template/adl1.4/{template_id}/example", params={"format": format}, ) response.raise_for_status() return response.json()Then use it in the router:
- client = await ehrbase_client._get_client() - response = await client.get( - f"/openehr/v1/definition/template/adl1.4/{template_id}/example", - params={"format": "FLAT"}, - ) - if response.status_code == 200: - return { - "template_id": template_id, - "format": "FLAT", - "example": response.json(), - } - raise HTTPException(status_code=404, detail="Template not found") + example = await ehrbase_client.get_template_example(template_id, "FLAT") + return { + "template_id": template_id, + "format": "FLAT", + "example": example, + }api/src/observations/service.py (3)
61-70: Consider moving logging import to module level and using timezone-aware datetime.Two minor improvements:
Inline import (line 66): Move
import loggingto the module's top-level imports for consistency and slight performance benefit.Deprecated
datetime.utcnow()(lines 64, 70): This is deprecated in Python 3.12+. Consider using timezone-aware UTC:🔎 Suggested changes
At module level:
from datetime import datetime +from datetime import timezone from typing import Any +import loggingIn the code:
- composition_uid = f"placeholder-{datetime.utcnow().isoformat()}" - # Log the error but don't fail - import logging - - logging.warning(f"EHRBase composition creation failed: {e}") + composition_uid = f"placeholder-{datetime.now(timezone.utc).isoformat()}" + logging.warning(f"EHRBase composition creation failed: {e}") - now = datetime.utcnow() + now = datetime.now(timezone.utc)
143-187: In-memory pagination may not scale for patients with many readings.Line 145 fetches all rows from EHRBase and then slices in memory:
rows[skip : skip + limit]. For patients with extensive vital signs history, this could:
- Transfer large amounts of data unnecessarily
- Increase memory usage
- Slow down response times
Consider pushing pagination to the AQL query using
LIMITandOFFSETclauses if EHRBase supports them.🔎 Example approach
If AQL supports pagination:
# In queries.py, parameterize the query: VITAL_SIGNS_QUERY_PAGINATED = """ SELECT ... FROM EHR e ... WHERE ... ORDER BY ... LIMIT $limit OFFSET $skip """ # Then in service: params = {"ehr_id": ehr_id, "limit": limit, "skip": skip}This moves pagination to the database layer for better performance.
189-226: Same encapsulation concern: direct access to_get_client().Both
delete_vital_signsandget_raw_compositionaccessehrbase_client._get_client()directly. Consider adding public methods toehrbase_clientfor these operations:
delete_composition(ehr_id, composition_uid)get_composition_with_format(ehr_id, composition_uid, format)This would improve maintainability and keep EHRBase API details encapsulated.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
web/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
api/pyproject.tomlapi/src/ehrbase/queries.pyapi/src/observations/router.pyapi/src/observations/schemas.pyapi/src/observations/service.pydocs/prd/0004-vital-signs-chart.mddocs/prd/README.mdweb/package.jsonweb/src/components/openehr/OpenEHRMetadataPanel.vueweb/src/components/vitals/RecordVitalsDialog.vueweb/src/components/vitals/VitalSignsChart.vueweb/src/components/vitals/VitalSignsPanel.vueweb/src/components/vitals/VitalSignsTable.vueweb/src/pages/patients/PatientDetailPage.vueweb/src/stores/vitals.tsweb/src/types/index.tsweb/src/types/vitals.ts
🧰 Additional context used
📓 Path-based instructions (5)
web/src/{pages,stores,composables,types,lib}/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Frontend structure should be organized with pages/ for Vue route components, stores/ for Pinia state management, composables/ for reusable composition functions, types/ for TypeScript definitions, and lib/ for UI component library (shadcn-vue)
Files:
web/src/stores/vitals.tsweb/src/types/vitals.tsweb/src/types/index.tsweb/src/pages/patients/PatientDetailPage.vue
web/src/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
TypeScript strict mode is enabled and implicit any types are not allowed in frontend code
Files:
web/src/stores/vitals.tsweb/src/components/vitals/VitalSignsTable.vueweb/src/components/vitals/VitalSignsPanel.vueweb/src/components/vitals/RecordVitalsDialog.vueweb/src/types/vitals.tsweb/src/types/index.tsweb/src/components/vitals/VitalSignsChart.vueweb/src/pages/patients/PatientDetailPage.vueweb/src/components/openehr/OpenEHRMetadataPanel.vue
api/src/ehrbase/{client,compositions,templates,queries}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use EHRBase integration modules organized under api/src/ehrbase/ with client.py for REST API client, compositions.py for composition helpers, templates.py for template management, and queries.py for AQL query builders
Files:
api/src/ehrbase/queries.py
api/src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
api/src/**/*.py: Type hints are required everywhere in Python code and enforced by mypy configuration
All functions in Python must be async
Use httpx.AsyncClient for async HTTP requests in Python; never use the synchronous requests library
Files:
api/src/ehrbase/queries.pyapi/src/observations/router.pyapi/src/observations/schemas.pyapi/src/observations/service.py
api/src/*/{router,service,repository,schemas}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Service Layer Pattern in Python API with separate modules for router.py (FastAPI route definitions), service.py (business logic layer), repository.py (Prisma database operations), and schemas.py (Pydantic models)
Files:
api/src/observations/router.pyapi/src/observations/schemas.pyapi/src/observations/service.py
🧠 Learnings (1)
📚 Learning: 2026-01-02T20:46:00.330Z
Learnt from: CR
Repo: platzhersh/open-cis PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:46:00.330Z
Learning: Applies to api/src/ehrbase/{client,compositions,templates,queries}.py : Use EHRBase integration modules organized under api/src/ehrbase/ with client.py for REST API client, compositions.py for composition helpers, templates.py for template management, and queries.py for AQL query builders
Applied to files:
api/src/ehrbase/queries.pyapi/src/observations/service.py
🧬 Code graph analysis (5)
web/src/stores/vitals.ts (2)
web/src/types/vitals.ts (6)
VitalSignsReading(22-32)VitalSignsListResponse(43-46)VitalSignsCreate(34-41)RawComposition(48-52)CompositionPathsResponse(60-64)ArchetypeInfo(66-73)web/src/lib/api.ts (1)
api(40-52)
web/src/types/vitals.ts (1)
api/src/observations/schemas.py (4)
VitalSignsCreate(29-55)VitalSignsListResponse(72-76)TemplateInfo(87-92)TemplateListResponse(95-98)
api/src/observations/router.py (3)
api/src/observations/schemas.py (6)
RawCompositionResponse(79-84)TemplateInfo(87-92)TemplateListResponse(95-98)VitalSignsCreate(29-55)VitalSignsListResponse(72-76)VitalSignsResponse(58-69)api/src/observations/service.py (5)
record_vital_signs(39-86)get_vital_signs_for_patient(108-187)get_vital_signs(88-106)delete_vital_signs(189-202)get_raw_composition(204-226)api/src/ehrbase/client.py (2)
list_templates(108-113)_get_client(15-29)
api/src/observations/schemas.py (1)
web/src/types/vitals.ts (4)
VitalSignsCreate(34-41)VitalSignsListResponse(43-46)TemplateInfo(75-79)TemplateListResponse(81-83)
api/src/observations/service.py (5)
api/src/observations/schemas.py (5)
OpenEHRMetadataResponse(19-26)PathMappingResponse(8-16)VitalSignsCreate(29-55)VitalSignsListResponse(72-76)VitalSignsResponse(58-69)web/src/types/vitals.ts (2)
VitalSignsCreate(34-41)VitalSignsListResponse(43-46)api/src/patients/repository.py (1)
find_patient_by_id(6-8)api/src/observations/router.py (4)
record_vital_signs(28-37)get_vital_signs(62-70)delete_vital_signs(74-81)get_raw_composition(139-153)api/src/ehrbase/client.py (4)
create_composition(65-84)get_composition(86-91)execute_aql(93-106)_get_client(15-29)
🪛 markdownlint-cli2 (0.18.1)
docs/prd/0004-vital-signs-chart.md
69-69: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
231-231: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
243-243: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
354-354: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
399-399: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
436-436: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
511-511: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
520-520: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (33)
docs/prd/README.md (1)
10-10: LGTM! Documentation entry properly formatted.The PRD index entry follows the established format and clearly describes the new feature.
api/pyproject.toml (1)
39-40: LGTM! Appropriate Ruff configuration for FastAPI.This configuration correctly tells the flake8-bugbear linter that FastAPI's
Query,Depends, andPathare safe to use as default arguments, preventing false positive warnings.api/src/ehrbase/queries.py (3)
21-39: Date range query looks correct, same OR clause caveat applies.The date range filtering logic is properly implemented using composition start_time. The same consideration about the
ORclause behavior applies here as well (see previous comment on lines 13-16).
65-72: LGTM! Count query correctly filters by template ID.The count query properly filters vital signs compositions using the template ID. The hardcoded template ID
'open-cis.vital-signs.v1'should remain stable in production.
13-16: No action needed. TheCONTAINS (bp OR pulse)clause correctly reflects the data model design, which intentionally allows vital signs to be recorded separately (either BP alone, pulse alone, or both in the same composition). The schema validation confirms this intent—all vital fields are optional, with the only requirement being that systolic and diastolic must be provided together if BP is recorded. The backend service properly handles NULL values through conditional type conversion (int(value) if value else None), and the response schema correctly defines all vital fields asint | None.web/src/types/index.ts (1)
82-83: LGTM! Standard barrel export pattern.Re-exporting vitals types through the index file follows TypeScript best practices for module organization.
web/package.json (1)
17-18: The specified versions of chart.js (4.5.1), chartjs-adapter-date-fns (3.0.0), and vue-chartjs (5.3.3) are current and have no known security vulnerabilities.web/src/pages/patients/PatientDetailPage.vue (2)
39-43: LGTM!The computed property correctly handles the null case for
store.currentPatientand assembles the full name appropriately for the child component.
383-389: LGTM!Clean integration of the new VitalSignsPanel component with proper prop passing. The section is well-structured within its own bordered container.
web/src/components/vitals/VitalSignsTable.vue (2)
75-113: LGTM!The table rendering is well-structured with proper conditional styling, null value handling using the em-dash fallback, and a clean empty state message.
11-13: No action needed. The emit event name casing is correct:showOpenEHR(camelCase) properly converts to@show-open-e-h-r(kebab-case) in Vue templates, and the parent component (VitalSignsPanel.vue) correctly listens with the kebab-case variant. TypeScript strict mode is properly followed with explicit type annotations.web/src/components/vitals/RecordVitalsDialog.vue (2)
50-63: LGTM on validation logic.The validation correctly ensures blood pressure completeness (both systolic and diastolic together or neither) and requires at least one vital sign.
71-78: LGTM on API payload construction.The nullish coalescing pattern (
?? null) correctly convertsundefinedform values tonullfor the API, matching the expectedVitalSignsCreateschema.web/src/components/vitals/VitalSignsChart.vue (3)
22-32: Chart.js registration at module level.Registering Chart.js components at module level is a common pattern, but be aware this is a side effect that runs on import. This is acceptable for this use case.
102-159: LGTM on chart configuration.The BP chart options are well-configured with proper time scale, tooltips, click handling, and reasonable y-axis bounds. The interaction mode
'index'withintersect: falseprovides good UX for selecting data points.
219-223: LGTM on data presence checks.The
hasBPDataandhasPulseDatacomputed properties correctly check for non-null values before rendering the respective charts.web/src/components/openehr/OpenEHRMetadataPanel.vue (1)
70-80: LGTM on clipboard utility.The copy-to-clipboard implementation with visual feedback and auto-reset after 2 seconds provides good UX.
web/src/components/vitals/VitalSignsPanel.vue (1)
156-167: LGTM on chart and table integration.The chart and table components are properly wired with event handlers for reading selection and openEHR metadata display.
web/src/types/vitals.ts (2)
1-46: LGTM! Type definitions are well-structured and align with backend schemas.The interfaces properly mirror the Pydantic models from
api/src/observations/schemas.py. Good use of nullable types (| null) matching the backend's optional fields.
85-113: Well-implemented clinical helper functions.The BP status logic correctly implements a simplified hypertension classification. The thresholds align with general guidelines (normal <120/80, high ≥140/90).
One minor note: clinically, "elevated" BP is typically defined as systolic 120-129 with diastolic <80, while values 130-139/80-89 are "Stage 1 hypertension." The current simplified approach works well for a basic UI indicator.
web/src/stores/vitals.ts (4)
1-18: Well-structured Pinia store setup.Good use of the composition API pattern with properly typed state refs.
20-43: LGTM! Proper async state management with error handling.The
fetchVitalsaction correctly manages loading state and handles errors gracefully.
45-59: LGTM! Record action is well-implemented.Correctly prepends new readings to maintain chronological order (most recent first) and returns the result for potential caller use.
122-148: LGTM! Clean utility functions and proper store exports.The store is well-organized with all necessary state and actions properly exposed.
api/src/observations/router.py (3)
1-19: LGTM! Clean router setup with proper imports.Good organization with async functions and type hints as required by coding guidelines.
22-82: Well-implemented CRUD endpoints with proper validation.Good use of FastAPI's
Querywith descriptions for documentation, appropriate HTTP status codes (201 for create, 204 for delete), and consistent error handling.
190-232: Good static archetype info with reasonable fallbacks.The hardcoded archetype information provides useful clinical context. The CKM links are valuable for transparency.
Minor note: Line 231's parsing
parts[0].split("-")[-1]safely handles missing "-" (returnsparts[0]itself), and the fallback to "UNKNOWN" is appropriate.api/src/observations/schemas.py (3)
1-27: Well-defined openEHR metadata schemas.Clean Pydantic models with appropriate typing. The
PathMappingResponsecorrectly uses union typefloat | int | str | Noneto handle various value types from openEHR compositions.
29-56: LGTM! Robust validation for vital signs input.The
model_validatorcorrectly ensures:
- At least one vital sign must be provided
- Blood pressure readings must be complete (both systolic and diastolic together)
The XOR check
(self.systolic is None) != (self.diastolic is None)elegantly catches partial BP submissions.
58-98: LGTM! Response schemas are well-structured.The schemas align with the frontend TypeScript interfaces and provide comprehensive data for both UI display and openEHR transparency.
api/src/observations/service.py (3)
1-37: Well-organized service with clear archetype mappings.Good use of class constants for template and archetype configuration. The archetype paths are properly defined for openEHR transparency. Based on learnings, this aligns with the project's EHRBase integration pattern.
88-106: LGTM! Clean single-record retrieval with graceful fallback.The method correctly handles missing patients and EHRBase errors by returning
None, allowing the router to respond with appropriate 404s.
318-367: Composition parser is simple but functional.The
_parse_composition_to_responsemethod provides a basic parser for FLAT format compositions. The comment on line 327 acknowledges it's simplified.Note: The pattern
"rate|magnitude" in key(line 339) could match other rate measurements if the template evolves. Consider using a more specific path like"pulse/any_event/rate|magnitude"for robustness if additional rate observations are added later.
The old CKM link (1013.1.170) was deprecated and replaced by 1013.1.4295.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
api/src/observations/router.py (1)
190-232: Consider moving hard-coded mappings to a constants module.The hard-coded dictionaries for CKM links and archetype descriptions (lines 205-223) make the code harder to maintain and extend. Consider extracting these to a dedicated constants file or configuration.
🔎 Proposed refactor
Create a new constants file:
# api/src/observations/openehr_constants.py """openEHR archetype metadata and CKM links.""" ARCHETYPE_CKM_LINKS = { "openEHR-EHR-OBSERVATION.blood_pressure.v2": "https://ckm.openehr.org/ckm/archetypes/1013.1.3574", "openEHR-EHR-OBSERVATION.pulse.v2": "https://ckm.openehr.org/ckm/archetypes/1013.1.4295", "openEHR-EHR-COMPOSITION.encounter.v1": "https://ckm.openehr.org/ckm/archetypes/1013.1.1366", } ARCHETYPE_DESCRIPTIONS = { "openEHR-EHR-OBSERVATION.blood_pressure.v2": ( "The local systemic arterial blood pressure which is a surrogate " "for arterial pressure in the systemic circulation." ), "openEHR-EHR-OBSERVATION.pulse.v2": ( "The rate and associated attributes for a pulse or heart beat." ), "openEHR-EHR-COMPOSITION.encounter.v1": ( "Interaction, contact or care event between a subject of care " "and healthcare provider(s)." ), }Then import and use in the router:
+from src.observations.openehr_constants import ARCHETYPE_CKM_LINKS, ARCHETYPE_DESCRIPTIONS @router.get("/openehr/archetypes/{archetype_id}") async def get_archetype_info(archetype_id: str) -> dict: """Get information about an archetype. Provides archetype details and link to Clinical Knowledge Manager (CKM). """ # Parse archetype ID to construct CKM URL parts = archetype_id.split(".") if len(parts) >= 2: concept = parts[-2] else: concept = archetype_id - # Known archetype mappings - ckm_links = {...} - descriptions = {...} return { "archetype_id": archetype_id, "concept": concept, - "description": descriptions.get(archetype_id, "No description available"), - "ckm_url": ckm_links.get(archetype_id), + "description": ARCHETYPE_DESCRIPTIONS.get(archetype_id, "No description available"), + "ckm_url": ARCHETYPE_CKM_LINKS.get(archetype_id), "reference_model": "EHR" if "EHR-" in archetype_id else "DEMOGRAPHIC", "type": parts[0].split("-")[-1] if "-" in parts[0] else "UNKNOWN", }This improves maintainability and makes it easier to extend support for additional archetypes.
docs/prd/0004-vital-signs-chart.md (1)
69-69: Add language specifiers to fenced code blocks for better rendering.Multiple fenced code blocks are missing language specifiers, which can affect rendering in some Markdown processors. Consider adding appropriate language identifiers to improve documentation quality.
🔎 Suggested language specifiers
- Line 69 (openEHR structure diagram): Add
textidentifier- Line 231 (API endpoints): Add
textorhttpidentifier- Line 243 (openEHR endpoints): Add
textorhttpidentifier- Lines 354, 399, 436, 511, 520 (UI mockups): Add
textidentifier for ASCII artExample fix for line 69:
```text COMPOSITION (openEHR-EHR-COMPOSITION.encounter.v1) ├── context ... ```This helps ensure consistent rendering across different Markdown processors and improves accessibility.
Based on learnings from static analysis tools.
Also applies to: 231-231, 243-243, 354-354, 399-399, 436-436, 511-511, 520-520
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/src/observations/router.pydocs/prd/0004-vital-signs-chart.md
🧰 Additional context used
📓 Path-based instructions (2)
api/src/*/{router,service,repository,schemas}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Service Layer Pattern in Python API with separate modules for router.py (FastAPI route definitions), service.py (business logic layer), repository.py (Prisma database operations), and schemas.py (Pydantic models)
Files:
api/src/observations/router.py
api/src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
api/src/**/*.py: Type hints are required everywhere in Python code and enforced by mypy configuration
All functions in Python must be async
Use httpx.AsyncClient for async HTTP requests in Python; never use the synchronous requests library
Files:
api/src/observations/router.py
🪛 markdownlint-cli2 (0.18.1)
docs/prd/0004-vital-signs-chart.md
69-69: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
231-231: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
243-243: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
354-354: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
399-399: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
436-436: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
511-511: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
520-520: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (7)
api/src/observations/router.py (7)
1-19: LGTM! Clean import structure.The imports follow the Service Layer Pattern correctly, separating router, schemas, and service concerns. Type hints are properly imported.
27-38: LGTM! Proper error handling and delegation.The endpoint correctly delegates business logic to the service layer and handles errors appropriately.
40-58: LGTM! Well-defined query parameters.The endpoint properly defines pagination and filtering parameters with appropriate validation constraints.
61-82: LGTM! Proper CRUD operations.Both endpoints follow best practices with appropriate error handling and 404 responses when resources are not found.
89-108: LGTM! Proper service integration.The endpoint correctly integrates with the EHRBase client and provides appropriate error handling with 503 status for service unavailability.
138-154: LGTM! Good use of Literal type.The endpoint properly constrains the format parameter using
Literal["FLAT", "STRUCTURED"], providing type safety and clear API documentation.
156-188: LGTM! Clear path extraction logic.The endpoint correctly extracts and formats composition paths for transparency, providing useful debugging information.
The CKM link 1013.1.1366 does not exist. Updated to the correct link 1013.1.120 for openEHR-EHR-COMPOSITION.encounter.v1.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
api/src/observations/router.py (1)
110-136: Refactor to avoid accessing private method.This endpoint directly accesses
ehrbase_client._get_client(), which breaks encapsulation. The issue has already been identified in a previous review with a recommended solution to add a public method to the EHRBase client.
🧹 Nitpick comments (2)
docs/prd/0004-vital-signs-chart.md (1)
69-69: Consider adding language specifiers to fenced code blocks.Several fenced code blocks lack language identifiers, which can affect rendering in some Markdown viewers. For plain text content (UI mockups, endpoint lists), you can add
textas the language specifier.Example fix
For ASCII art UI mockups and plain text lists:
-``` +```text ┌─────────────────────────────────────────┐ │ UI Mockup │ └─────────────────────────────────────────┘For endpoint lists: ```diff -``` +```text POST /api/observations/vital-signs GET /api/observations/vital-signs</details> Also applies to: 231-231, 243-243, 354-354, 399-399, 436-436, 511-511, 520-520 </blockquote></details> <details> <summary>api/src/observations/router.py (1)</summary><blockquote> `205-223`: **Consider externalizing archetype mappings to a constant or configuration.** The hardcoded dictionaries for CKM links and descriptions work for now but could be moved to module-level constants or a configuration file for easier maintenance as more archetypes are added. <details> <summary>Example refactor</summary> At the module level: ```python # Archetype metadata registry ARCHETYPE_REGISTRY = { "openEHR-EHR-OBSERVATION.blood_pressure.v2": { "ckm_url": "https://ckm.openehr.org/ckm/archetypes/1013.1.3574", "description": "The local systemic arterial blood pressure which is a surrogate for arterial pressure in the systemic circulation.", }, "openEHR-EHR-OBSERVATION.pulse.v2": { "ckm_url": "https://ckm.openehr.org/ckm/archetypes/1013.1.4295", "description": "The rate and associated attributes for a pulse or heart beat.", }, "openEHR-EHR-COMPOSITION.encounter.v1": { "ckm_url": "https://ckm.openehr.org/ckm/archetypes/1013.1.120", "description": "Interaction, contact or care event between a subject of care and healthcare provider(s).", }, }Then in the function:
archetype_data = ARCHETYPE_REGISTRY.get(archetype_id, {}) return { "archetype_id": archetype_id, "concept": concept, "description": archetype_data.get("description", "No description available"), "ckm_url": archetype_data.get("ckm_url"), "reference_model": "EHR" if "EHR-" in archetype_id else "DEMOGRAPHIC", "type": parts[0].split("-")[-1] if "-" in parts[0] else "UNKNOWN", }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/src/observations/router.pydocs/prd/0004-vital-signs-chart.md
🧰 Additional context used
📓 Path-based instructions (2)
api/src/*/{router,service,repository,schemas}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Service Layer Pattern in Python API with separate modules for router.py (FastAPI route definitions), service.py (business logic layer), repository.py (Prisma database operations), and schemas.py (Pydantic models)
Files:
api/src/observations/router.py
api/src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
api/src/**/*.py: Type hints are required everywhere in Python code and enforced by mypy configuration
All functions in Python must be async
Use httpx.AsyncClient for async HTTP requests in Python; never use the synchronous requests library
Files:
api/src/observations/router.py
🪛 markdownlint-cli2 (0.18.1)
docs/prd/0004-vital-signs-chart.md
69-69: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
231-231: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
243-243: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
354-354: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
399-399: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
436-436: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
511-511: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
520-520: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (3)
api/src/observations/router.py (3)
1-17: LGTM: Clean imports with proper type hints support.The imports are well-organized and include all necessary types (datetime, Literal, Query) for comprehensive type annotations, adhering to the coding guidelines.
22-82: LGTM: Well-structured CRUD endpoints following best practices.The vital signs endpoints properly:
- Use the service layer pattern
- Include comprehensive type hints
- Implement appropriate HTTP status codes (201 for creation, 204 for deletion)
- Validate query parameters (pagination limits, required fields)
- Handle errors with proper HTTP exceptions
89-107: LGTM: openEHR transparency endpoints are well-implemented.The template listing, raw composition, and path extraction endpoints follow consistent patterns with:
- Proper delegation to service/client layers
- Comprehensive error handling with appropriate HTTP status codes
- Type hints throughout
- Clear documentation
Also applies to: 138-187
Vital signs must now be linked to an encounter, aligning with clinical workflow and openEHR data modeling best practices. Changes: - Make encounter_id required in VitalSignsCreate schema (backend) - Update VitalSignsReading and VitalSignsCreate types (frontend) - Add encounter selector dropdown to RecordVitalsDialog - Auto-select encounter if only one active encounter exists - Show message if no active encounters are available
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
web/src/components/vitals/RecordVitalsDialog.vue (2)
190-196: Missing future date validation (duplicate of previous review).The datetime-local input still lacks the
maxattribute to prevent future dates, as noted in the previous review. Per the PRD,recorded_at"Cannot be in future". Themaxattribute should be set to the current local datetime.🔎 Proposed fix
Add a computed property for the max value and apply it to the input:
+const maxDateTime = computed(() => { + const now = new Date() + const pad = (n: number) => n.toString().padStart(2, '0') + return `${now.getFullYear()}-${pad(now.getMonth() + 1)}-${pad(now.getDate())}T${pad(now.getHours())}:${pad(now.getMinutes())}` +})Then in the template:
<input id="recorded-at" v-model="recordedAt" type="datetime-local" required + :max="maxDateTime" class="flex h-10 flex-1 rounded-md border border-input bg-background px-3 py-2 text-sm ring-offset-background placeholder:text-muted-foreground focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2" />
102-129: Add client-side validation to prevent future dates.The submit handler correctly builds and sends the payload, but it lacks validation to ensure
recordedAtis not in the future, as required by the PRD. While the datetime-local input issue is noted separately, the submit handler should also perform this check before sending the request.🔎 Proposed fix
async function handleSubmit() { if (!canSubmit.value) return + // Validate that recorded_at is not in the future + const recordedDate = new Date(recordedAt.value) + if (recordedDate > new Date()) { + formError.value = 'Date and time cannot be in the future' + return + } + formError.value = null submitting.value = true
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/src/observations/schemas.pyweb/src/components/vitals/RecordVitalsDialog.vueweb/src/types/vitals.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/types/vitals.ts
🧰 Additional context used
📓 Path-based instructions (3)
web/src/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
TypeScript strict mode is enabled and implicit any types are not allowed in frontend code
Files:
web/src/components/vitals/RecordVitalsDialog.vue
api/src/*/{router,service,repository,schemas}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Service Layer Pattern in Python API with separate modules for router.py (FastAPI route definitions), service.py (business logic layer), repository.py (Prisma database operations), and schemas.py (Pydantic models)
Files:
api/src/observations/schemas.py
api/src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
api/src/**/*.py: Type hints are required everywhere in Python code and enforced by mypy configuration
All functions in Python must be async
Use httpx.AsyncClient for async HTTP requests in Python; never use the synchronous requests library
Files:
api/src/observations/schemas.py
🧬 Code graph analysis (1)
api/src/observations/schemas.py (1)
web/src/types/vitals.ts (4)
VitalSignsCreate(34-41)VitalSignsListResponse(43-46)TemplateInfo(75-79)TemplateListResponse(81-83)
🔇 Additional comments (14)
web/src/components/vitals/RecordVitalsDialog.vue (8)
1-7: LGTM!The imports are well-organized, importing the necessary Vue 3 composition utilities, Radix Vue dialog components, icons, stores, and TypeScript types.
23-39: LGTM!Form state initialization and encounter filtering logic are correct. The filter appropriately identifies active encounters by checking for 'in-progress' or 'planned' status.
42-64: LGTM!The dialog initialization logic is well-structured. The auto-selection of a single active encounter is a nice UX touch, and the async encounter fetching is properly handled with loading states.
66-70: LGTM!The
setNowfunction correctly formats the current datetime to match thedatetime-localinput value format.
72-77: LGTM!The encounter label formatting provides clear, user-friendly information combining date, type, and reason.
79-100: LGTM!The validation logic is well-structured and defensive:
bpIsCompletecorrectly checks for bothundefinedandnull(whichv-model.numbercan produce)hasAtLeastOneVitalensures at least one measurement is recordedcanSubmitproperly combines all validations and prevents double submission
131-133: LGTM!Simple and correct event propagation for dialog state changes.
136-189: LGTM!The dialog UI is well-structured with:
- Proper Radix Vue Dialog components
- Clear loading and empty states for encounter selection
- Appropriate input constraints matching the backend schema (systolic: 50-300, diastolic: 30-200, pulse: 20-300)
- Helpful validation messages
- Well-styled action buttons with loading indicators
Also applies to: 197-298
api/src/observations/schemas.py (6)
1-6: LGTM!Module docstring and imports are appropriate. The addition of
model_validatorenables custom validation logic for the schemas.
8-16: LGTM!The
PathMappingResponseschema is well-defined for mapping UI fields to OpenEHR paths, supporting the transparency feature.
19-26: LGTM!The
OpenEHRMetadataResponseschema correctly encapsulates OpenEHR metadata for transparency, including composition UID, template ID, archetype IDs, EHR ID, and path mappings.
37-55: LGTM!The Field constraints properly match the frontend input ranges, and the model validator correctly ensures:
- At least one vital sign is provided
- BP measurements are complete (both systolic and diastolic together)
The XOR logic on line 52 elegantly validates BP pair completeness.
58-69: LGTM!The
VitalSignsResponseschema is well-structured, including all vital sign fields plus OpenEHR metadata for transparency. The use ofcomposition_uidas theidfield maintains consistency with OpenEHR standards.
72-98: LGTM!The remaining schemas are clean and well-defined:
VitalSignsListResponseprovides pagination supportRawCompositionResponseenables OpenEHR transparencyTemplateInfoandTemplateListResponsesupport template discoveryAll schemas are properly typed and serve their intended purposes.
- Add automatic template registration on API startup - Check if required templates exist in EHRBase and upload missing ones - Create templates/ directory for OPT files - Add upload_templates.py script for manual template management - Add ADR-0001 documenting template handling approach Note: The included OPT file (open-cis.vital-signs.v1.opt) is a minimal skeleton. A proper template should be created using openEHR tooling (Ocean Template Designer, Better Archetype Designer, or ADL Designer) with full archetype constraints embedded.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
api/src/ehrbase/templates.py (1)
38-45: Consider more specific error handlingReturning an empty list on any exception may hide legitimate errors (e.g., authentication failures, network issues) that differ from "no templates found". Consider logging different exception types or re-raising critical errors.
🔎 Proposed refinement
async def get_registered_template_ids() -> list[str]: """Get list of template IDs registered in EHRBase.""" try: templates = await list_templates() return [t.get("template_id", "") for t in templates] + except httpx.ConnectError as e: + logger.error(f"Cannot connect to EHRBase: {e}") + raise except Exception as e: logger.warning(f"Failed to list EHRBase templates: {e}") return []docs/adr/0001-openehr-template-management.md (1)
45-47: Minor markdown formatting improvementsThe static analysis tool flagged several markdown formatting issues:
- Lines 45-47, 95-96: Bare URLs should use proper link syntax
[text](url)- Line 81: Code fence should specify language (use
```textor```bash)🔎 Proposed fixes
-1. **Ocean Template Designer** (free): https://tools.openehr.org/designer/ -2. **Better Archetype Designer**: https://tools.better.care/ -3. **ADL Designer**: https://tools.openehr.org/designer/ +1. **Ocean Template Designer** (free): <https://tools.openehr.org/designer/> +2. **Better Archetype Designer**: <https://tools.better.care/> +3. **ADL Designer**: <https://tools.openehr.org/designer/> -``` +```text api/ ├── templates/ │ └── open-cis.vital-signs.v1.opt # OPT template file -- EHRBase documentation: https://ehrbase.readthedocs.io/ -- openEHR CKM: https://ckm.openehr.org/ +- EHRBase documentation: <https://ehrbase.readthedocs.io/> +- openEHR CKM: <https://ckm.openehr.org/>Also applies to: 81-81, 95-96
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
api/scripts/upload_templates.pyapi/src/ehrbase/templates.pyapi/src/main.pyapi/templates/open-cis.vital-signs.v1.optdocs/adr/0001-openehr-template-management.md
🧰 Additional context used
📓 Path-based instructions (2)
api/src/ehrbase/{client,compositions,templates,queries}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use EHRBase integration modules organized under api/src/ehrbase/ with client.py for REST API client, compositions.py for composition helpers, templates.py for template management, and queries.py for AQL query builders
Files:
api/src/ehrbase/templates.py
api/src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
api/src/**/*.py: Type hints are required everywhere in Python code and enforced by mypy configuration
All functions in Python must be async
Use httpx.AsyncClient for async HTTP requests in Python; never use the synchronous requests library
Files:
api/src/ehrbase/templates.pyapi/src/main.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: platzhersh/open-cis PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:46:00.330Z
Learning: Applies to api/src/ehrbase/{client,compositions,templates,queries}.py : Use EHRBase integration modules organized under api/src/ehrbase/ with client.py for REST API client, compositions.py for composition helpers, templates.py for template management, and queries.py for AQL query builders
📚 Learning: 2026-01-02T20:46:00.330Z
Learnt from: CR
Repo: platzhersh/open-cis PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:46:00.330Z
Learning: Applies to api/src/ehrbase/{client,compositions,templates,queries}.py : Use EHRBase integration modules organized under api/src/ehrbase/ with client.py for REST API client, compositions.py for composition helpers, templates.py for template management, and queries.py for AQL query builders
Applied to files:
api/src/ehrbase/templates.pyapi/scripts/upload_templates.pyapi/src/main.py
🧬 Code graph analysis (2)
api/scripts/upload_templates.py (1)
api/src/ehrbase/templates.py (2)
list_templates(17-19)upload_template(22-24)
api/src/main.py (1)
api/src/ehrbase/templates.py (1)
ensure_templates_registered(64-106)
🪛 markdownlint-cli2 (0.18.1)
docs/adr/0001-openehr-template-management.md
45-45: Bare URL used
(MD034, no-bare-urls)
46-46: Bare URL used
(MD034, no-bare-urls)
47-47: Bare URL used
(MD034, no-bare-urls)
81-81: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
95-95: Bare URL used
(MD034, no-bare-urls)
96-96: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (7)
api/src/main.py (2)
11-11: LGTM: Import added correctlyThe import of
ensure_templates_registeredfollows the established project structure for EHRBase integration modules.
28-37: LGTM: Graceful template registration on startupThe startup template registration logic is well-implemented with appropriate error handling and logging. The graceful degradation approach (continuing startup even if template registration fails) aligns with the ADR-0001 decision.
api/templates/open-cis.vital-signs.v1.opt (1)
1-220: LGTM: Well-formed OpenEHR templateThe template follows OpenEHR OPT 1.4 standards and correctly defines the vital signs structure with blood pressure and pulse observations. The template ID matches the reference in
REQUIRED_TEMPLATES.api/src/ehrbase/templates.py (1)
3-4: LGTM: Module setup and constantsThe logging setup and
REQUIRED_TEMPLATESconstant are well-organized and follow the template management approach described in ADR-0001.Also applies to: 9-14
api/scripts/upload_templates.py (3)
1-20: Script setup follows common patternThe shebang, docstring, and imports are appropriate for a utility script. The
sys.pathmanipulation (line 18) is a common pattern for standalone scripts, though it's a code smell in production code.
23-29: LGTM: Template listing helperThe function correctly fetches and extracts template IDs, with appropriate fallback to empty list on non-200 responses.
61-113: LGTM: Well-structured main function with proper error handlingThe main function includes:
- Connectivity checks before proceeding
- Clear progress reporting
- Graceful handling of check-only mode
- Appropriate exit codes
The simple argument parsing (line 117) is acceptable for a utility script.
Replace minimal placeholder template with proper OPT from RippleOSI: - Download IDCR - Vital Signs Encounter.v1.opt from Ripple-openEHR repo - Update service to use v1 archetype versions (blood_pressure.v1, pulse.v1) - Update FLAT composition paths to match template structure - Support both v1 and v2 archetype versions in router CKM links Template source: https://github.com/RippleOSI/Ripple-openEHR
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
api/src/ehrbase/templates.py (2)
49-62: Fragile HTTP 409 detection using string matching.Line 58 detects "already exists" by checking if
"409"appears anywhere in the error message string. This is fragile and may yield false positives if "409" appears in unrelated error text.Based on the relevant code snippet from
api/src/ehrbase/client.py, theupload_template()method useshttpx.AsyncClientand callsresponse.raise_for_status(), which raiseshttpx.HTTPStatusError. Check the status code directly:🔎 Proposed fix using proper exception inspection
+import httpx + async def upload_template_file(template_id: str, template_content: str) -> bool: """Upload a single template to EHRBase.""" try: await upload_template(template_content) logger.info(f"Template {template_id} uploaded successfully") return True + except httpx.HTTPStatusError as e: + if e.response.status_code == 409: + logger.info(f"Template {template_id} already exists") + return True + logger.error(f"Failed to upload template {template_id}: HTTP {e.response.status_code}") + return False except Exception as e: - error_msg = str(e) - # 409 means template already exists - that's OK - if "409" in error_msg: - logger.info(f"Template {template_id} already exists") - return True logger.error(f"Failed to upload template {template_id}: {e}") return False
65-107: Blocking file I/O in async function.Line 104 uses synchronous
read_text()which blocks the event loop. Use async file I/O viaaiofilesfor better concurrency.🔎 Proposed fix using aiofiles
First, add
aiofilesto your dependencies. Then:+import aiofiles + async def ensure_templates_registered() -> dict[str, bool]: """ Ensure all required templates are registered in EHRBase. Called during API startup. Returns a dict mapping template_id to success status. """ results: dict[str, bool] = {} # Find templates directory (relative to this file) templates_dir = Path(__file__).parent.parent.parent / "templates" if not templates_dir.exists(): logger.warning(f"Templates directory not found: {templates_dir}") return results # Check what's already registered try: existing = await get_registered_template_ids() logger.info(f"EHRBase has {len(existing)} registered template(s)") except Exception as e: logger.warning(f"Could not connect to EHRBase to check templates: {e}") return results # Upload any missing required templates for template_id in REQUIRED_TEMPLATES: template_file = templates_dir / f"{template_id}.opt" if not template_file.exists(): logger.warning(f"Template file not found: {template_file}") results[template_id] = False continue if template_id in existing: logger.info(f"Template {template_id} already registered") results[template_id] = True continue # Read and upload logger.info(f"Uploading template {template_id}...") - template_content = template_file.read_text() + async with aiofiles.open(template_file, mode='r') as f: + template_content = await f.read() results[template_id] = await upload_template_file(template_id, template_content) return resultsapi/src/observations/router.py (1)
110-135: Refactor to avoid accessing private method.Line 118 accesses the private method
ehrbase_client._get_client(), which breaks encapsulation. The codebase already has a public function for this purpose inapi/src/ehrbase/templates.py(lines 28-36).🔎 Recommended approach
Use the existing public function from the templates module:
+from src.ehrbase.templates import get_template_example + @router.get("/openehr/templates/{template_id}") async def get_template_info(template_id: str) -> dict: """Get detailed information about a template. Returns the template structure with example paths. """ try: - # Get template example to show structure - client = await ehrbase_client._get_client() - response = await client.get( - f"/openehr/v1/definition/template/adl1.4/{template_id}/example", - params={"format": "FLAT"}, - ) - if response.status_code == 200: - return { - "template_id": template_id, - "format": "FLAT", - "example": response.json(), - } - raise HTTPException(status_code=404, detail="Template not found") + example = await get_template_example(template_id, format="FLAT") + return { + "template_id": template_id, + "format": "FLAT", + "example": example, + } except HTTPException: raise except Exception as e: raise HTTPException( status_code=503, detail=f"Failed to fetch template: {e}" ) from e
🧹 Nitpick comments (2)
api/src/observations/router.py (1)
190-241: Consider externalizing archetype mappings.The hardcoded CKM links and descriptions work well for the current use case. However, as the number of supported archetypes grows, consider moving these mappings to a configuration file or database table for easier maintenance.
api/src/observations/service.py (1)
39-86: Move logging import to module level.Lines 66-68 import
logginginside the function and uselogging.warning()instead of the module-levellogger. This is inconsistent with the rest of the codebase.🔎 Proposed fix
Add
import loggingat the top of the file and use the module-level logger:+import logging + from datetime import datetime from typing import Any from src.ehrbase.client import ehrbase_clientThen update the usage:
except Exception as e: # If EHRBase is unavailable or template not uploaded, return with placeholder # This allows development without EHRBase running composition_uid = f"placeholder-{datetime.utcnow().isoformat()}" # Log the error but don't fail - import logging - - logging.warning(f"EHRBase composition creation failed: {e}") + logger.warning(f"EHRBase composition creation failed: {e}")Add the logger at module level after imports:
from src.patients.repository import find_patient_by_id +logger = logging.getLogger(__name__) + class ObservationService:
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/src/ehrbase/templates.pyapi/src/observations/router.pyapi/src/observations/service.pyapi/templates/IDCR - Vital Signs Encounter.v1.opt
🧰 Additional context used
📓 Path-based instructions (3)
api/src/ehrbase/{client,compositions,templates,queries}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use EHRBase integration modules organized under api/src/ehrbase/ with client.py for REST API client, compositions.py for composition helpers, templates.py for template management, and queries.py for AQL query builders
Files:
api/src/ehrbase/templates.py
api/src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
api/src/**/*.py: Type hints are required everywhere in Python code and enforced by mypy configuration
All functions in Python must be async
Use httpx.AsyncClient for async HTTP requests in Python; never use the synchronous requests library
Files:
api/src/ehrbase/templates.pyapi/src/observations/router.pyapi/src/observations/service.py
api/src/*/{router,service,repository,schemas}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Service Layer Pattern in Python API with separate modules for router.py (FastAPI route definitions), service.py (business logic layer), repository.py (Prisma database operations), and schemas.py (Pydantic models)
Files:
api/src/observations/router.pyapi/src/observations/service.py
🧠 Learnings (1)
📚 Learning: 2026-01-02T20:46:00.330Z
Learnt from: CR
Repo: platzhersh/open-cis PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:46:00.330Z
Learning: Applies to api/src/ehrbase/{client,compositions,templates,queries}.py : Use EHRBase integration modules organized under api/src/ehrbase/ with client.py for REST API client, compositions.py for composition helpers, templates.py for template management, and queries.py for AQL query builders
Applied to files:
api/src/ehrbase/templates.pyapi/src/observations/service.py
🧬 Code graph analysis (2)
api/src/ehrbase/templates.py (3)
api/src/observations/router.py (1)
list_templates(90-107)api/scripts/upload_templates.py (2)
list_templates(23-29)upload_template(32-58)api/src/ehrbase/client.py (2)
list_templates(108-113)upload_template(115-124)
api/src/observations/service.py (3)
api/src/observations/schemas.py (5)
OpenEHRMetadataResponse(19-26)PathMappingResponse(8-16)VitalSignsCreate(29-55)VitalSignsListResponse(72-76)VitalSignsResponse(58-69)api/src/patients/repository.py (1)
find_patient_by_id(6-8)api/src/ehrbase/client.py (4)
create_composition(65-84)get_composition(86-91)execute_aql(93-106)_get_client(15-29)
🪛 GitHub Actions: Backend CI
api/src/observations/service.py
[error] 167-167: Argument "encounter_id" to "VitalSignsResponse" has incompatible type "None"; expected "str" [arg-type]
[error] 176-176: Missing named argument "encounter_id" for "VitalSignsCreate" [call-arg]
[error] 355-355: Argument "encounter_id" to "VitalSignsResponse" has incompatible type "None"; expected "str" [arg-type]
[error] 364-364: Missing named argument "encounter_id" for "VitalSignsCreate" [call-arg]
🔇 Additional comments (11)
api/src/ehrbase/templates.py (2)
3-15: LGTM!The imports, logger setup, and REQUIRED_TEMPLATES constant are well-structured and documented.
39-46: LGTM!The function properly handles errors by logging and returning an empty list, which allows graceful degradation when EHRBase is unavailable.
api/src/observations/router.py (5)
1-25: LGTM!The imports are well-organized, and the section headers improve code readability.
27-58: LGTM!The CRUD endpoints are well-structured with proper type hints, error handling, and comprehensive query parameters for filtering and pagination.
61-81: LGTM!The get and delete endpoints follow REST conventions with appropriate status codes and error handling.
89-107: LGTM!The template listing endpoint properly handles errors with a 503 status code when EHRBase is unavailable.
138-187: LGTM!The composition transparency endpoints provide comprehensive path mapping and support multiple formats with proper type validation using
Literal.api/src/observations/service.py (4)
1-37: LGTM!The archetype metadata structure is well-organized and provides comprehensive path mappings for openEHR transparency.
88-106: LGTM!The single vital signs retrieval method properly handles errors and delegates parsing to a helper method.
228-265: LGTM!The FLAT composition builder correctly constructs the openEHR structure with proper paths, units, and conditional inclusion of vitals data.
267-321: LGTM!The metadata builder provides comprehensive openEHR transparency by mapping UI fields to both archetype paths and FLAT paths with proper units.
| def _parse_composition_to_response( | ||
| self, | ||
| composition: dict[str, Any], | ||
| composition_uid: str, | ||
| patient_id: str, | ||
| ehr_id: str, | ||
| ) -> VitalSignsResponse: | ||
| """Parse a raw composition to VitalSignsResponse.""" | ||
| # Extract values from composition (depends on format) | ||
| # This is a simplified parser - real implementation would handle STRUCTURED format | ||
| recorded_at = datetime.utcnow() | ||
| systolic = None | ||
| diastolic = None | ||
| pulse_rate = None | ||
|
|
||
| # Try to extract from FLAT format paths | ||
| for key, value in composition.items(): | ||
| if "systolic|magnitude" in key: | ||
| systolic = int(value) if value else None | ||
| elif "diastolic|magnitude" in key: | ||
| diastolic = int(value) if value else None | ||
| elif "rate|magnitude" in key: | ||
| pulse_rate = int(value) if value else None | ||
| elif "start_time" in key: | ||
| try: | ||
| recorded_at = datetime.fromisoformat(str(value).replace("Z", "+00:00")) | ||
| except ValueError: | ||
| pass | ||
|
|
||
| return VitalSignsResponse( | ||
| id=composition_uid, | ||
| patient_id=patient_id, | ||
| encounter_id=None, | ||
| recorded_at=recorded_at, | ||
| systolic=systolic, | ||
| diastolic=diastolic, | ||
| pulse_rate=pulse_rate, | ||
| created_at=recorded_at, | ||
| openehr_metadata=self._build_openehr_metadata( | ||
| composition_uid=composition_uid, | ||
| ehr_id=ehr_id, | ||
| data=VitalSignsCreate( | ||
| patient_id=patient_id, | ||
| recorded_at=recorded_at, | ||
| systolic=systolic, | ||
| diastolic=diastolic, | ||
| pulse_rate=pulse_rate, | ||
| ), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Fix encounter_id type mismatch causing pipeline failures.
Lines 355 and 364 create instances with encounter_id=None, causing the same mypy errors as in get_vital_signs_for_patient. This is the same root cause: the schemas require encounter_id: str but the parser doesn't have access to this value from the raw composition.
The fix is the same as suggested for lines 108-187: make encounter_id optional in both VitalSignsCreate and VitalSignsResponse schemas. Then add encounter_id=None to line 364:
openehr_metadata=self._build_openehr_metadata(
composition_uid=composition_uid,
ehr_id=ehr_id,
data=VitalSignsCreate(
patient_id=patient_id,
+ encounter_id=None,
recorded_at=recorded_at,
systolic=systolic,
diastolic=diastolic,
pulse_rate=pulse_rate,
),
),Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Backend CI
[error] 355-355: Argument "encounter_id" to "VitalSignsResponse" has incompatible type "None"; expected "str" [arg-type]
[error] 364-364: Missing named argument "encounter_id" for "VitalSignsCreate" [call-arg]
🤖 Prompt for AI Agents
In api/src/observations/service.py around lines 323 to 372, the parser builds
VitalSignsResponse and VitalSignsCreate with encounter_id=None but the schemas
require encounter_id: str which causes mypy/pipeline failures; update the
VitalSignsCreate and VitalSignsResponse schemas to make encounter_id
Optional[str] (i.e. allow None), then in this function include encounter_id=None
explicitly when constructing the VitalSignsResponse (and include
encounter_id=None in the nested VitalSignsCreate passed to
_build_openehr_metadata) so types align and mypy errors are resolved.
- Update PRD-0004 with IDCR template details from Ripple-openEHR - Change archetype versions from v2 to v1 throughout - Update FLAT path examples with section-based structure - Make encounter_id required in data model and UI - Mark template-related decisions as resolved - Link PRD to ADR-0001 for template management details
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/adr/0001-openehr-template-management.md (1)
48-48: Fix markdown style issues: wrap bare URLs and add code block language identifiers.The markdownlint static analysis flagged several style issues that should be corrected for consistency and improved rendering:
- Bare URLs (lines 48, 102–104, 149–151): Wrap URLs in markdown link syntax
[text](url)or use inline references- Code block language specs (lines 75, 92, 135): Add language identifiers after opening fence (e.g.,
```python,```yaml)🔎 Proposed fixes for markdown style issues
-Source**: https://github.com/RippleOSI/Ripple-openEHR/blob/master/technical/operational/IDCR%20-%20Vital%20Signs%20Encounter.opt +Source**: [RippleOSI Ripple-openEHR](https://github.com/RippleOSI/Ripple-openEHR/blob/master/technical/operational/IDCR%20-%20Vital%20Signs%20Encounter.opt) -``` +```python -2. **Ocean Template Designer** (free): https://tools.openehr.org/designer/ -3. **Better Archetype Designer**: https://tools.better.care/ -4. **ADL Designer**: https://tools.openehr.org/designer/ +2. **Ocean Template Designer** (free): [https://tools.openehr.org/designer/](https://tools.openehr.org/designer/) +3. **Better Archetype Designer**: [https://tools.better.care/](https://tools.better.care/) +4. **ADL Designer**: [https://tools.openehr.org/designer/](https://tools.openehr.org/designer/) -``` +``` api/ -EHRBase documentation: https://ehrbase.readthedocs.io/ -openEHR CKM: https://ckm.openehr.org/ -Template source: https://github.com/RippleOSI/Ripple-openEHR +EHRBase documentation: [https://ehrbase.readthedocs.io/](https://ehrbase.readthedocs.io/) +openEHR CKM: [https://ckm.openehr.org/](https://ckm.openehr.org/) +Template source: [RippleOSI Ripple-openEHR](https://github.com/RippleOSI/Ripple-openEHR)Also applies to: 75-75, 92-92, 102-104, 135-135, 149-151
docs/prd/0004-vital-signs-chart.md (1)
70-70: Add language specifiers to code blocks and fix bare URL formatting.The markdown linter flagged several formatting issues:
- Bare URL (Line 70): Ensure the GitHub URL is wrapped in markdown link syntax (e.g.,
[text](url))- Code block language specs (Lines 79, 105–119, 244–263, 366–442, 448–516, 523–537): Add language identifiers to fenced code blocks:
- JSON examples → use
```json- HTTP requests → use
```httpor```plaintext- ASCII diagrams → use
```plaintextor```text🔎 Example fixes for code blocks
JSON example (around Line 105):
-``` +```json { "ctx/language": "en",HTTP requests (around Line 244):
-``` +```http POST /api/observations/vital-signsASCII diagrams (around Line 366):
-``` +```plaintext ┌─────────────────────────────────────────────────────────────────┐Also applies to: 79-79, 105-119, 244-263, 366-442, 448-516, 523-537
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/adr/0001-openehr-template-management.mddocs/prd/0004-vital-signs-chart.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
docs/adr/0001-openehr-template-management.md
48-48: Bare URL used
(MD034, no-bare-urls)
75-75: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
92-92: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
102-102: Bare URL used
(MD034, no-bare-urls)
103-103: Bare URL used
(MD034, no-bare-urls)
104-104: Bare URL used
(MD034, no-bare-urls)
135-135: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
149-149: Bare URL used
(MD034, no-bare-urls)
150-150: Bare URL used
(MD034, no-bare-urls)
151-151: Bare URL used
(MD034, no-bare-urls)
docs/prd/0004-vital-signs-chart.md
70-70: Bare URL used
(MD034, no-bare-urls)
79-79: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
244-244: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
256-256: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
366-366: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
411-411: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
448-448: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
523-523: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
532-532: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (1)
docs/adr/0001-openehr-template-management.md (1)
1-151: Well-structured ADR with clear decision rationale and good template sourcing.The document provides comprehensive coverage of the template management decision, including context, decision, consequences, and implementation guidance. The choice of the IDCR Vital Signs template is well-justified, and the acknowledgment of v1 vs v2 archetype trade-offs shows thoughtful decision-making for a learning project.
- Use lowercase 'application/xml' for Content-Type - Add Accept header supporting both JSON and XML responses - Handle non-JSON responses from EHRBase on template upload
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/src/ehrbase/client.py
🧰 Additional context used
📓 Path-based instructions (2)
api/src/ehrbase/{client,compositions,templates,queries}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use EHRBase integration modules organized under api/src/ehrbase/ with client.py for REST API client, compositions.py for composition helpers, templates.py for template management, and queries.py for AQL query builders
Files:
api/src/ehrbase/client.py
api/src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
api/src/**/*.py: Type hints are required everywhere in Python code and enforced by mypy configuration
All functions in Python must be async
Use httpx.AsyncClient for async HTTP requests in Python; never use the synchronous requests library
Files:
api/src/ehrbase/client.py
🧠 Learnings (1)
📚 Learning: 2026-01-02T20:46:00.330Z
Learnt from: CR
Repo: platzhersh/open-cis PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:46:00.330Z
Learning: Applies to api/src/ehrbase/{client,compositions,templates,queries}.py : Use EHRBase integration modules organized under api/src/ehrbase/ with client.py for REST API client, compositions.py for composition helpers, templates.py for template management, and queries.py for AQL query builders
Applied to files:
api/src/ehrbase/client.py
🔇 Additional comments (1)
api/src/ehrbase/client.py (1)
121-124: LGTM: Correct headers for XML template upload.The Content-Type and Accept headers are appropriate for uploading operational templates (OPT files) to EHRBase. The XML Content-Type correctly reflects the template content format, and accepting both JSON and XML responses provides flexibility for different EHRBase server configurations.
| # EHRBase may return empty response or XML on success | ||
| if response.headers.get("content-type", "").startswith("application/json"): | ||
| return response.json() | ||
| return {"status": "uploaded"} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the context around lines 127-130 in client.py
cat -n api/src/ehrbase/client.py | sed -n '115,145p'Repository: platzhersh/open-cis
Length of output: 1135
🌐 Web query:
EHRBase API template upload POST /openehr/v1/definition/template/adl1.4 response format documentation
💡 Result:
POST /openehr/v1/definition/template/adl1.4 — response format (summary)
- Success: 201 Created. Server SHOULD return the server-assigned template_id in the Location response header (optionally with version). An ETag header MAY be present. Depending on the Prefer header the response body is either empty (minimal) or contains the full template representation (OPERATIONAL_TEMPLATE in XML/JSON). Errors: 400 for invalid content, 409 if a template with same id/version exists. (specifications.openehr.org)
Citations:
Consider handling the actual response data instead of a synthetic fallback.
Per OpenEHR API specification, the template upload endpoint correctly returns 201 Created with response body that is either empty (minimal preference) or contains the template representation in XML or JSON depending on content negotiation and Prefer header. The defensive check for application/json content-type is appropriate.
However, the fallback {"status": "uploaded"} is a synthetic response not returned by the server. When the response body is empty or XML, return the actual response data or relevant headers (the Location header contains the server-assigned template_id), rather than a constructed dict that doesn't represent the actual server response.
🤖 Prompt for AI Agents
In api/src/ehrbase/client.py around lines 127 to 130, the code returns a
synthetic {"status": "uploaded"} when the server response is empty or non-JSON;
replace this fallback with the actual response data and relevant headers: if
Content-Type is application/json return response.json(); if Content-Type is
application/xml or text/xml return the raw response.text (or parse to an XML
object if desired); if body is empty, return a minimal representation containing
response.status_code and any relevant headers (e.g. Location for template_id)
instead of a fabricated status field; ensure no synthetic dict is returned so
callers can inspect real server output and headers.
- Always log "Checking openEHR templates..." on startup - Log when no templates are configured - Add summary line showing template check results
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
api/src/ehrbase/templates.py (2)
55-62: Fragile HTTP 409 detection via string matchingThis concern was already raised in a previous review. The string-based check
"409" in error_msgis fragile. Prefer catchinghttpx.HTTPStatusErrorand inspectinge.response.status_code == 409directly.
110-110: Blocking file I/O in async contextThis concern was already raised in a previous review. The synchronous
read_text()call blocks the event loop. Useaiofilesfor async file reading.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/src/ehrbase/templates.py
🧰 Additional context used
📓 Path-based instructions (2)
api/src/ehrbase/{client,compositions,templates,queries}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use EHRBase integration modules organized under api/src/ehrbase/ with client.py for REST API client, compositions.py for composition helpers, templates.py for template management, and queries.py for AQL query builders
Files:
api/src/ehrbase/templates.py
api/src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
api/src/**/*.py: Type hints are required everywhere in Python code and enforced by mypy configuration
All functions in Python must be async
Use httpx.AsyncClient for async HTTP requests in Python; never use the synchronous requests library
Files:
api/src/ehrbase/templates.py
🧠 Learnings (1)
📚 Learning: 2026-01-02T20:46:00.330Z
Learnt from: CR
Repo: platzhersh/open-cis PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:46:00.330Z
Learning: Applies to api/src/ehrbase/{client,compositions,templates,queries}.py : Use EHRBase integration modules organized under api/src/ehrbase/ with client.py for REST API client, compositions.py for composition helpers, templates.py for template management, and queries.py for AQL query builders
Applied to files:
api/src/ehrbase/templates.py
🧬 Code graph analysis (1)
api/src/ehrbase/templates.py (3)
api/src/ehrbase/client.py (2)
list_templates(108-113)upload_template(115-130)api/src/observations/router.py (1)
list_templates(90-107)api/scripts/upload_templates.py (2)
list_templates(23-29)upload_template(32-58)
🪛 GitHub Actions: Backend CI
api/src/ehrbase/templates.py
[error] 119-119: Ruff check failed: E501 Line too long (103 > 100). Command: 'ruff check .'
🔇 Additional comments (2)
api/src/ehrbase/templates.py (2)
3-15: LGTM!Clean setup with appropriate imports, standard logger configuration, and well-documented constant for required templates.
39-46: LGTM!Clean async wrapper with appropriate error handling and logging. The pattern matches the existing
list_templateshelper in the upload scripts.
…ions, and make encounter ID optional in schemas.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
api/src/observations/schemas.py (1)
34-34: Add validation to prevent future dates.The
recorded_atfield lacks validation to ensure it's not in the future. Per the PRD requirements, vital signs "Cannot be in future". The backend should authoritatively enforce this constraint.🔎 Proposed fix
+from datetime import timezone + class VitalSignsCreate(BaseModel): """Request to record vital signs.""" patient_id: str encounter_id: str | None = None recorded_at: datetime + + @field_validator("recorded_at") + @classmethod + def validate_not_future(cls, v: datetime) -> datetime: + """Ensure recorded_at is not in the future.""" + now = datetime.now(timezone.utc) + if v > now: + raise ValueError("recorded_at cannot be in the future") + return vNote: Use timezone-aware comparison since timestamps are likely stored in UTC.
api/src/observations/service.py (2)
189-202: Refactor to avoid accessing private method.Line 196 accesses the private method
ehrbase_client._get_client(), which breaks encapsulation. Add a publicdelete_composition()method to the EHRBase client instead.🔎 Recommended approach
Add a public method to
api/src/ehrbase/client.py:async def delete_composition(self, ehr_id: str, composition_uid: str) -> bool: """Delete a composition by UID.""" client = await self._get_client() response = await client.delete( f"/openehr/v1/ehr/{ehr_id}/composition/{composition_uid}" ) return response.status_code == 204Then update this method:
async def delete_vital_signs(self, composition_uid: str, patient_id: str) -> bool: """Delete a vital signs composition.""" patient = await find_patient_by_id(patient_id) if not patient: return False try: - client = await ehrbase_client._get_client() - response = await client.delete( - f"/openehr/v1/ehr/{patient.ehrId}/composition/{composition_uid}" - ) - return response.status_code == 204 + return await ehrbase_client.delete_composition(patient.ehrId, composition_uid) except Exception: return FalseBased on coding guidelines and past review feedback.
204-226: Refactor to use or extend existing public method.Line 213 accesses the private method
ehrbase_client._get_client(). The EHRBase client already has a publicget_composition()method (api/src/ehrbase/client.py lines 85-90), but it doesn't support theformatparameter.🔎 Recommended approach
Extend the existing public method in
api/src/ehrbase/client.py:-async def get_composition(self, ehr_id: str, composition_uid: str) -> dict[str, Any]: +async def get_composition( + self, + ehr_id: str, + composition_uid: str, + format: str | None = None +) -> dict[str, Any]: """Get a composition by UID.""" client = await self._get_client() - response = await client.get(f"/openehr/v1/ehr/{ehr_id}/composition/{composition_uid}") + params = {"format": format} if format else {} + response = await client.get( + f"/openehr/v1/ehr/{ehr_id}/composition/{composition_uid}", + params=params + ) response.raise_for_status() return response.json()Then update this method:
async def get_raw_composition( self, composition_uid: str, patient_id: str, format: str = "FLAT" ) -> dict[str, Any] | None: """Get raw composition data for transparency.""" patient = await find_patient_by_id(patient_id) if not patient: return None try: - client = await ehrbase_client._get_client() - response = await client.get( - f"/openehr/v1/ehr/{patient.ehrId}/composition/{composition_uid}", - params={"format": format}, - ) - if response.status_code == 200: - return { - "format": format, - "template_id": self.TEMPLATE_ID, - "composition": response.json(), - } + composition = await ehrbase_client.get_composition( + ehr_id=patient.ehrId, + composition_uid=composition_uid, + format=format + ) + return { + "format": format, + "template_id": self.TEMPLATE_ID, + "composition": composition, + } except Exception: - pass - return None + return NoneBased on coding guidelines and past review feedback.
🧹 Nitpick comments (5)
api/src/main.py (1)
26-26: Add return type annotation to thelifespanfunction.The
lifespanfunction is missing a return type annotation. As per coding guidelines, type hints are required everywhere.🔎 Suggested fix
+from typing import AsyncIterator + @asynccontextmanager -async def lifespan(app: FastAPI): +async def lifespan(app: FastAPI) -> AsyncIterator[None]: # Startup - attempt database connection but don't fail if unavailableapi/src/observations/service.py (4)
66-68: Move import to module level.The
import loggingstatement is placed inside the function. Per Python best practices, imports should be at the module level for clarity and to avoid repeated import overhead.🔎 Proposed fix
Move the import to the top of the file:
+import logging from datetime import datetime from typing import AnyThen remove line 66 inside the function.
64-64: Replace deprecated datetime.utcnow() with timezone-aware alternative.Lines 64 and 70 use
datetime.utcnow(), which is deprecated in Python 3.12+. Usedatetime.now(timezone.utc)for timezone-aware timestamps.🔎 Proposed fix
+from datetime import datetime, timezone -from datetime import datetime from typing import Any # In record_vital_signs method: - composition_uid = f"placeholder-{datetime.utcnow().isoformat()}" + composition_uid = f"placeholder-{datetime.now(timezone.utc).isoformat()}" - now = datetime.utcnow() + now = datetime.now(timezone.utc)Also applies to: 70-70
157-157: Consider timezone-aware datetime and adding explicit encounter_id parameter.Line 157 uses
datetime.utcnow()(deprecated). Line 176 omitsencounter_idparameter, relying on the default None. While valid, explicitly passingencounter_id=Noneimproves clarity.🔎 Proposed improvements
+from datetime import datetime, timezone -from datetime import datetime try: recorded_at = datetime.fromisoformat( recorded_at_str.replace("Z", "+00:00") ) except (ValueError, AttributeError): - recorded_at = datetime.utcnow() + recorded_at = datetime.now(timezone.utc) openehr_metadata=self._build_openehr_metadata( composition_uid=composition_uid, ehr_id=ehr_id, data=VitalSignsCreate( patient_id=patient_id, + encounter_id=None, recorded_at=recorded_at, systolic=int(systolic) if systolic else None, diastolic=int(diastolic) if diastolic else None, pulse_rate=int(pulse_rate) if pulse_rate else None, ), ),Also applies to: 176-176
340-340: Consider timezone-aware datetime and explicit parameter passing.Lines 340 and 355 use
datetime.utcnow()(deprecated). Line 373 omitsencounter_idparameter. While valid, explicit parameters improve code clarity.🔎 Proposed improvements
+from datetime import datetime, timezone -from datetime import datetime def _parse_composition_to_response( self, composition: dict[str, Any], composition_uid: str, patient_id: str, ehr_id: str, ) -> VitalSignsResponse: """Parse a raw composition to VitalSignsResponse.""" # Extract values from composition (depends on format) # This is a simplified parser - real implementation would handle STRUCTURED format - recorded_at = datetime.utcnow() + recorded_at = datetime.now(timezone.utc) systolic = None diastolic = None pulse_rate = None # Try to extract from FLAT format paths for key, value in composition.items(): if "systolic|magnitude" in key: systolic = int(value) if value else None elif "diastolic|magnitude" in key: diastolic = int(value) if value else None elif "rate|magnitude" in key: pulse_rate = int(value) if value else None elif "start_time" in key: try: - recorded_at = datetime.fromisoformat(str(value).replace("Z", "+00:00")) + recorded_at = datetime.fromisoformat(str(value).replace("Z", "+00:00")) except ValueError: pass return VitalSignsResponse( id=composition_uid, patient_id=patient_id, encounter_id=None, recorded_at=recorded_at, systolic=systolic, diastolic=diastolic, pulse_rate=pulse_rate, created_at=recorded_at, openehr_metadata=self._build_openehr_metadata( composition_uid=composition_uid, ehr_id=ehr_id, data=VitalSignsCreate( patient_id=patient_id, + encounter_id=None, recorded_at=recorded_at, systolic=systolic, diastolic=diastolic, pulse_rate=pulse_rate, ), ), )Also applies to: 355-355, 373-373
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/src/ehrbase/queries.pyapi/src/main.pyapi/src/observations/schemas.pyapi/src/observations/service.py
🧰 Additional context used
📓 Path-based instructions (3)
api/src/*/{router,service,repository,schemas}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Service Layer Pattern in Python API with separate modules for router.py (FastAPI route definitions), service.py (business logic layer), repository.py (Prisma database operations), and schemas.py (Pydantic models)
Files:
api/src/observations/schemas.pyapi/src/observations/service.py
api/src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
api/src/**/*.py: Type hints are required everywhere in Python code and enforced by mypy configuration
All functions in Python must be async
Use httpx.AsyncClient for async HTTP requests in Python; never use the synchronous requests library
Files:
api/src/observations/schemas.pyapi/src/main.pyapi/src/observations/service.pyapi/src/ehrbase/queries.py
api/src/ehrbase/{client,compositions,templates,queries}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use EHRBase integration modules organized under api/src/ehrbase/ with client.py for REST API client, compositions.py for composition helpers, templates.py for template management, and queries.py for AQL query builders
Files:
api/src/ehrbase/queries.py
🧠 Learnings (1)
📚 Learning: 2026-01-02T20:46:00.330Z
Learnt from: CR
Repo: platzhersh/open-cis PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:46:00.330Z
Learning: Applies to api/src/ehrbase/{client,compositions,templates,queries}.py : Use EHRBase integration modules organized under api/src/ehrbase/ with client.py for REST API client, compositions.py for composition helpers, templates.py for template management, and queries.py for AQL query builders
Applied to files:
api/src/main.pyapi/src/observations/service.pyapi/src/ehrbase/queries.py
🧬 Code graph analysis (3)
api/src/observations/schemas.py (1)
web/src/types/vitals.ts (4)
VitalSignsCreate(34-41)VitalSignsListResponse(43-46)TemplateInfo(75-79)TemplateListResponse(81-83)
api/src/main.py (1)
api/src/ehrbase/templates.py (1)
ensure_templates_registered(65-121)
api/src/observations/service.py (4)
api/src/observations/schemas.py (5)
OpenEHRMetadataResponse(19-26)PathMappingResponse(8-16)VitalSignsCreate(29-55)VitalSignsListResponse(72-76)VitalSignsResponse(58-69)api/src/patients/repository.py (1)
find_patient_by_id(6-8)api/src/ehrbase/client.py (4)
create_composition(65-84)get_composition(86-91)execute_aql(93-106)_get_client(15-29)api/src/ehrbase/compositions.py (2)
create_composition(8-20)get_composition(23-25)
🔇 Additional comments (16)
api/src/main.py (3)
11-11: LGTM!The import is correct and aligns with the EHRBase module organization structure.
16-20: LGTM!Module-level logging configuration is appropriate for the application entry point. The format string is clear and the INFO level is suitable for production use.
34-43: Template registration logic looks good, but verify failure behavior.The implementation correctly:
- Awaits the async
ensure_templates_registered()function- Handles exceptions gracefully with appropriate logging
- Logs per-template status when results are available
The application continues startup even if template registration fails (warning-level logging). Verify this "best effort" approach is intentional for your deployment requirements. For production systems, you may want to fail startup if critical templates cannot be registered.
api/src/ehrbase/queries.py (2)
3-19: LGTM! Query structure supports optional vital signs.The OR clause correctly allows compositions with either blood pressure, pulse, or both. The service layer (lines 159-161 in service.py) properly handles NULL values for missing observations.
21-39: LGTM! Date range query is consistent with base query.The inclusive date range filters (>= and <=) are appropriate for vital signs retrieval.
api/src/observations/schemas.py (5)
8-26: LGTM! OpenEHR metadata schemas are well-structured.These schemas provide clear transparency into the openEHR composition structure and path mappings, supporting the ADR goals mentioned in the PR.
43-55: LGTM! Validator logic correctly enforces vital signs completeness.The model validator appropriately ensures:
- At least one vital sign is provided
- Blood pressure measurements are complete (both systolic and diastolic together)
58-69: LGTM! Response schema correctly handles optional encounter_id.Making
encounter_idoptional (line 63) appropriately reflects that AQL queries don't always return encounter context, resolving the type mismatches flagged in past reviews.
72-84: LGTM! List and raw composition schemas are correctly structured.These schemas align with the frontend TypeScript interfaces and support the transparency features described in the PR.
87-98: LGTM! Template schemas are correctly structured.The schemas appropriately model template metadata for the OpenEHR integration.
api/src/observations/service.py (6)
18-37: LGTM! OpenEHR archetype metadata is well-organized.The ARCHETYPES dictionary provides clear mappings for blood pressure and pulse observations, supporting the transparency features. Note: ensure TEMPLATE_ID matches what's used in VITAL_SIGNS_COUNT_QUERY (flagged in queries.py review).
88-106: LGTM! Single vital signs retrieval is correctly implemented.The method appropriately uses the public
ehrbase_client.get_composition()method and handles errors gracefully.
108-187: LGTM! AQL-based listing correctly handles optional fields.The method appropriately handles:
- Date range filtering with conditional query selection
- Parsing AQL results with NULL values for optional vital signs
- Optional
encounter_id(now supported by schemas)
228-272: LGTM! FLAT composition builder correctly maps vital signs to openEHR paths.The method appropriately constructs the FLAT format composition with:
- Required composition metadata (language, territory, category, context)
- Optional blood pressure observations with proper paths and units
- Optional pulse observations with proper paths and units
274-328: LGTM! OpenEHR metadata builder provides comprehensive transparency.The method correctly constructs metadata including:
- Archetype IDs for composition and observations
- Path mappings with archetype paths, FLAT paths, values, and units
- Conditional inclusion based on which vital signs are provided
330-379: LGTM! Composition parser correctly extracts vital signs from FLAT format.The parser appropriately:
- Iterates through composition keys to find vital sign values
- Handles FLAT path patterns for systolic, diastolic, and pulse_rate
- Extracts start_time with proper ISO format handling
- Constructs VitalSignsResponse with openEHR metadata
…ate handling to use `IDCR - Vital Signs Encounter.v1`.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
api/src/ehrbase/client.py (1)
121-130: Synthetic fallback still returns fabricated response data.The past review flagged that returning
{"status": "uploaded"}is a synthetic response not returned by the server. Consider returning the actual response data or relevant headers (e.g.,Locationheader contains the server-assignedtemplate_id).api/src/observations/service.py (2)
196-203: Refactor to avoid accessing private method_get_client().This still accesses
ehrbase_client._get_client()directly. Consider adding a publicdelete_compositionmethod to the EHRBase client as suggested in the past review.
213-227: Refactor to avoid accessing private method_get_client().This still accesses
ehrbase_client._get_client()directly. Consider extending the existing publicget_composition()method to support theformatparameter as suggested in the past review.
🧹 Nitpick comments (6)
api/src/observations/router.py (1)
198-225: Consider externalizing hardcoded archetype metadata.The
ckm_linksanddescriptionsdictionaries contain hardcoded data that may become stale or incomplete as more archetypes are added. Consider moving this to a configuration file or fetching from CKM API for maintainability.api/src/observations/service.py (4)
64-68: Avoid inline import and use UTC-aware datetime.The inline
import loggingis a code smell. Move this to the module-level imports. Also,datetime.utcnow()is deprecated in Python 3.12+; usedatetime.now(UTC)instead for consistency with other parts of this codebase.🔎 Proposed fix
Add at module level:
import logging from datetime import UTC, datetimeThen update the code:
- composition_uid = f"placeholder-{datetime.utcnow().isoformat()}" - # Log the error but don't fail - import logging - - logging.warning(f"EHRBase composition creation failed: {e}") + composition_uid = f"placeholder-{datetime.now(UTC).isoformat()}" + logger = logging.getLogger(__name__) + logger.warning(f"EHRBase composition creation failed: {e}")
70-70: Usedatetime.now(UTC)instead of deprecateddatetime.utcnow().
datetime.utcnow()is deprecated in Python 3.12+. Usedatetime.now(UTC)for consistency with the schemas module.🔎 Proposed fix
+from datetime import UTC, datetime + - now = datetime.utcnow() + now = datetime.now(UTC)
152-157: Usedatetime.now(UTC)instead of deprecateddatetime.utcnow().Same issue as above - use timezone-aware datetime for the fallback value.
🔎 Proposed fix
except (ValueError, AttributeError): - recorded_at = datetime.utcnow() + recorded_at = datetime.now(UTC)
341-341: Usedatetime.now(UTC)instead of deprecateddatetime.utcnow().Same issue - use timezone-aware datetime.
web/src/components/vitals/RecordVitalsDialog.vue (1)
66-70: Consider extracting date formatting logic.The date formatting logic (padding and template string) is duplicated between
setNowandmaxDateTime. While this works correctly, extracting it to a helper function would improve maintainability.🔎 Proposed refactor
+function formatDateTimeLocal(date: Date): string { + const pad = (n: number) => n.toString().padStart(2, '0') + return `${date.getFullYear()}-${pad(date.getMonth() + 1)}-${pad(date.getDate())}T${pad(date.getHours())}:${pad(date.getMinutes())}` +} + function setNow() { - const now = new Date() - const pad = (n: number) => n.toString().padStart(2, '0') - recordedAt.value = `${now.getFullYear()}-${pad(now.getMonth() + 1)}-${pad(now.getDate())}T${pad(now.getHours())}:${pad(now.getMinutes())}` + recordedAt.value = formatDateTimeLocal(new Date()) } const maxDateTime = computed(() => { - const now = new Date() - const pad = (n: number) => n.toString().padStart(2, '0') - return `${now.getFullYear()}-${pad(now.getMonth() + 1)}-${pad(now.getDate())}T${pad(now.getHours())}:${pad(now.getMinutes())}` + return formatDateTimeLocal(new Date()) })Also applies to: 101-105
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/pyproject.tomlapi/src/ehrbase/client.pyapi/src/ehrbase/queries.pyapi/src/ehrbase/templates.pyapi/src/observations/router.pyapi/src/observations/schemas.pyapi/src/observations/service.pyweb/src/components/vitals/RecordVitalsDialog.vueweb/src/components/vitals/VitalSignsPanel.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/components/vitals/VitalSignsPanel.vue
🧰 Additional context used
📓 Path-based instructions (4)
api/src/ehrbase/{client,compositions,templates,queries}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use EHRBase integration modules organized under api/src/ehrbase/ with client.py for REST API client, compositions.py for composition helpers, templates.py for template management, and queries.py for AQL query builders
Files:
api/src/ehrbase/client.pyapi/src/ehrbase/templates.pyapi/src/ehrbase/queries.py
api/src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
api/src/**/*.py: Type hints are required everywhere in Python code and enforced by mypy configuration
All functions in Python must be async
Use httpx.AsyncClient for async HTTP requests in Python; never use the synchronous requests library
Files:
api/src/ehrbase/client.pyapi/src/ehrbase/templates.pyapi/src/ehrbase/queries.pyapi/src/observations/schemas.pyapi/src/observations/router.pyapi/src/observations/service.py
web/src/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
TypeScript strict mode is enabled and implicit any types are not allowed in frontend code
Files:
web/src/components/vitals/RecordVitalsDialog.vue
api/src/*/{router,service,repository,schemas}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Service Layer Pattern in Python API with separate modules for router.py (FastAPI route definitions), service.py (business logic layer), repository.py (Prisma database operations), and schemas.py (Pydantic models)
Files:
api/src/observations/schemas.pyapi/src/observations/router.pyapi/src/observations/service.py
🧠 Learnings (1)
📚 Learning: 2026-01-02T20:46:00.330Z
Learnt from: CR
Repo: platzhersh/open-cis PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:46:00.330Z
Learning: Applies to api/src/ehrbase/{client,compositions,templates,queries}.py : Use EHRBase integration modules organized under api/src/ehrbase/ with client.py for REST API client, compositions.py for composition helpers, templates.py for template management, and queries.py for AQL query builders
Applied to files:
api/src/ehrbase/client.pyapi/src/ehrbase/templates.pyapi/src/observations/service.py
🧬 Code graph analysis (5)
api/src/ehrbase/client.py (1)
api/src/ehrbase/templates.py (1)
get_template_example(31-39)
api/src/ehrbase/templates.py (3)
api/src/ehrbase/client.py (2)
list_templates(108-113)upload_template(115-130)api/src/observations/router.py (1)
list_templates(90-107)api/scripts/upload_templates.py (2)
list_templates(23-29)upload_template(32-58)
api/src/observations/schemas.py (1)
web/src/types/vitals.ts (4)
VitalSignsCreate(34-41)VitalSignsListResponse(43-46)TemplateInfo(75-79)TemplateListResponse(81-83)
api/src/observations/router.py (3)
api/src/observations/schemas.py (6)
RawCompositionResponse(90-95)TemplateInfo(98-103)TemplateListResponse(106-109)VitalSignsCreate(29-66)VitalSignsListResponse(83-87)VitalSignsResponse(69-80)api/src/ehrbase/client.py (2)
list_templates(108-113)get_template_example(132-142)api/src/ehrbase/templates.py (2)
list_templates(21-23)get_template_example(31-39)
api/src/observations/service.py (2)
api/src/observations/schemas.py (5)
OpenEHRMetadataResponse(19-26)PathMappingResponse(8-16)VitalSignsCreate(29-66)VitalSignsListResponse(83-87)VitalSignsResponse(69-80)api/src/patients/repository.py (1)
find_patient_by_id(6-8)
🪛 GitHub Actions: Backend CI
api/src/ehrbase/templates.py
[error] 7-7: mypy check failed. Step: 'mypy .'. Library stubs not installed for 'aiofiles'. Hint: install types-aiofiles (python3 -m pip install types-aiofiles) or run 'mypy --install-types' to install missing stub packages.
🔇 Additional comments (13)
api/src/ehrbase/client.py (1)
132-142: LGTM - New method follows established patterns.The
get_template_examplemethod correctly uses async patterns, proper type hints, and follows the existing client API design. Good addition for openEHR transparency features.api/pyproject.toml (1)
40-41: Good addition for FastAPI linting.The
extend-immutable-callsconfiguration correctly marks FastAPI'sQuery,Depends, andPathas immutable, preventing false positives from flake8-bugbear's B008 rule about function calls in default arguments.api/src/ehrbase/queries.py (2)
65-72: Template ID mismatch has been fixed.The query now correctly uses
'IDCR - Vital Signs Encounter.v1'which matches the template ID inservice.pyandtemplates.py.
13-16: The CONTAINS OR syntax is appropriate for this use case. In openEHR AQL,CONTAINS (OBSERVATION a OR OBSERVATION b)correctly matches compositions containing at least one of the specified observation types (blood pressure, pulse, or both). This is the intended behavior for optional archetype-level observations and requires no changes.api/src/ehrbase/templates.py (2)
52-67: HTTP 409 detection properly refactored.Good improvement catching
httpx.HTTPStatusErrorand checkinge.response.status_code == 409directly instead of string matching. This is more robust and follows the recommendation from the past review.
115-121: Async file I/O properly implemented.Good use of
aiofiles.open()for non-blocking file reads, addressing the past review concern about blocking the event loop.api/src/observations/router.py (2)
110-128: Private method access properly refactored.Good improvement using the public
ehrbase_client.get_template_example()method instead of accessing_get_client()directly. This addresses the encapsulation concern from the past review.
89-107: LGTM - Template listing endpoint properly implemented.The endpoint correctly maps EHRBase response to
TemplateListResponseschema and handles external service failures with appropriate 503 status.api/src/observations/schemas.py (2)
43-52: Future date validation properly implemented.Good implementation with timezone awareness. The validator correctly handles both naive and timezone-aware datetimes by normalizing to UTC before comparison.
54-66: LGTM - Comprehensive vital signs validation.The model validator ensures business rules are enforced: at least one vital sign must be provided, and blood pressure fields must be complete (both systolic and diastolic together or neither).
api/src/observations/service.py (1)
108-188: LGTM - Comprehensive vital signs retrieval with proper pagination.The
get_vital_signs_for_patientmethod correctly handles date range filtering, AQL query execution, and response mapping. Good fallback behavior when EHRBase is unavailable.web/src/components/vitals/RecordVitalsDialog.vue (2)
1-21: LGTM!The imports, props, and emits are properly typed with explicit TypeScript types, complying with strict mode requirements.
92-113: Well implemented date validation!The future date validation requested in the previous review has been properly implemented with:
maxattribute to constrain the datetime pickerisDateTimeValidcomputed property checking recorded time ≤ nowvalidateDateTimefunction with user-facing error messages- Integration with
canSubmitto prevent invalid submissionsThis fully addresses the PRD requirement that recorded_at "Cannot be in future".
Also applies to: 214-237
| from pathlib import Path | ||
| from typing import Any | ||
|
|
||
| import aiofiles |
There was a problem hiding this comment.
Pipeline failure: Missing type stubs for aiofiles.
The mypy check is failing due to missing type stubs. This is addressed by adding types-aiofiles to dev dependencies in pyproject.toml.
🧰 Tools
🪛 GitHub Actions: Backend CI
[error] 7-7: mypy check failed. Step: 'mypy .'. Library stubs not installed for 'aiofiles'. Hint: install types-aiofiles (python3 -m pip install types-aiofiles) or run 'mypy --install-types' to install missing stub packages.
🤖 Prompt for AI Agents
In api/src/ehrbase/templates.py around line 7 the import of aiofiles triggers
mypy failure because type stubs are missing; add the package "types-aiofiles" to
the development dependencies in pyproject.toml (under
[tool.poetry.dev-dependencies] or equivalent) and then update the lock file
(poetry lock / pip-compile / pip install -r) so CI picks up the stubs; commit
both the pyproject.toml change and the updated lockfile.
| respiratory_rate: float | None = None | ||
| oxygen_saturation: float | None = None | ||
| recorded_at: datetime | None = None | ||
| encounter_id: str | None = None # Optional - vital signs can be recorded without encounter |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the frontend TypeScript type definition
echo "=== Frontend TypeScript Definition ==="
rg -n "encounter_id" web/src/types/vitals.ts -A 2 -B 2
# Check if the file exists and show its content
if [ -f web/src/types/vitals.ts ]; then
echo -e "\n=== Full vitals.ts content ==="
cat -n web/src/types/vitals.ts
fiRepository: platzhersh/open-cis
Length of output: 4077
🏁 Script executed:
#!/bin/bash
# Check the backend schema file
echo "=== Backend schemas.py content ==="
head -50 api/src/observations/schemas.py | cat -nRepository: platzhersh/open-cis
Length of output: 2026
Resolve type mismatch: encounter_id is optional in backend but required in frontend.
Backend schema defines encounter_id: str | None = None (optional), but web/src/types/vitals.ts defines encounter_id: string in both VitalSignsReading and VitalSignsCreate interfaces (required). Decide whether encounter_id should be optional or required, then align both backend and frontend accordingly.
🤖 Prompt for AI Agents
In api/src/observations/schemas.py around line 33, the backend marks
encounter_id as optional (encounter_id: str | None = None) while frontend types
expect a required string; pick one source of truth and align both sides. Option
A (make encounter_id required): remove the None union and default so backend
defines encounter_id: str and update any usages/validators to enforce presence;
then update tests and API docs. Option B (make frontend optional): change
web/src/types/vitals.ts so encounter_id is string | null | undefined (or
optional property) in VitalSignsReading and VitalSignsCreate and update
consuming code to handle missing encounter_id. Implement the chosen change
consistently across backend, frontend, and tests/validation.
| data=VitalSignsCreate( | ||
| patient_id=patient_id, | ||
| encounter_id=None, | ||
| recorded_at=recorded_at, | ||
| systolic=systolic, | ||
| diastolic=diastolic, | ||
| pulse_rate=pulse_rate, | ||
| ), |
There was a problem hiding this comment.
Potential validation bypass when parsing compositions with missing vitals.
When parsing a composition that has neither blood pressure nor pulse data, VitalSignsCreate is constructed with all vital fields as None. This will fail the model validator that requires "at least one vital sign must be provided". Consider adding a skip for validation or handling this edge case.
🔎 Proposed fix
Either skip validation when constructing from raw data:
data=VitalSignsCreate.model_construct(
patient_id=patient_id,
encounter_id=None,
recorded_at=recorded_at,
systolic=systolic,
diastolic=diastolic,
pulse_rate=pulse_rate,
)Or ensure at least one value exists before constructing:
# Only build metadata if we have valid vitals
if systolic is not None or diastolic is not None or pulse_rate is not None:
openehr_metadata = self._build_openehr_metadata(...)🤖 Prompt for AI Agents
In api/src/observations/service.py around lines 372 to 379, constructing
VitalSignsCreate with all vitals possibly None can trigger the model validator
that requires at least one vital; change the code to either (A) bypass
validation when creating from raw data by using the pydantic model_construct API
to build the object without running validators, or (B) guard construction by
checking that at least one of systolic, diastolic, or pulse_rate is not None and
only build/openehr_metadata when a value exists (return/skip otherwise). Ensure
you pick one approach and apply it consistently where compositions may lack
vitals.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
web/src/components/openehr/OpenEHRMetadataPanel.vue (3)
70-80: Consider cleaning up timeouts on unmount.The
setTimeoutcallback may fire after the component is unmounted, potentially causing a memory leak or attempting to update unmounted state. While Vue 3 typically handles this gracefully, it's best practice to explicitly clean up timers.🔎 Proposed fix using onUnmounted
-import { ref, watch, computed } from 'vue' +import { ref, watch, computed, onUnmounted } from 'vue' +const copyTimeouts: number[] = [] + +onUnmounted(() => { + copyTimeouts.forEach(id => window.clearTimeout(id)) +}) + async function copyToClipboard(text: string, identifier: string) { try { await navigator.clipboard.writeText(text) copiedPath.value = identifier - window.setTimeout(() => { + const timeoutId = window.setTimeout(() => { copiedPath.value = null }, 2000) + copyTimeouts.push(timeoutId) } catch (e) { console.error('Failed to copy:', e) } }
148-148: Remove redundant null coalescing operator.The
?? undefinedis unnecessary since thev-ifon line 147 already ensuresckm_urlis truthy, and the:hrefattribute acceptsstring | undefinednatively.🔎 Proposed fix
-:href="archetypeInfos.get(archetypeId)?.ckm_url ?? undefined" +:href="archetypeInfos.get(archetypeId)?.ckm_url"
223-261: Refactor duplicated tab content into a reusable component or template ref.The FLAT and STRUCTURED tabs have nearly identical rendering logic (lines 223-261), violating the DRY principle. Consider extracting this into a computed property or a reusable render function.
🔎 Proposed refactor approach
Extract the common rendering logic:
+const renderCompositionTab = () => ( + <div class="relative"> + <button + class="absolute right-2 top-2 inline-flex items-center justify-center rounded-md h-6 w-6 hover:bg-accent" + onClick={() => copyToClipboard(formattedComposition.value, 'composition')} + > + {copiedPath.value === 'composition' ? ( + <Check class="h-4 w-4 text-green-600" /> + ) : ( + <Copy class="h-4 w-4" /> + )} + </button> + {rawComposition.value && !loading.value ? ( + <pre class="rounded-lg border bg-muted/30 p-3 text-xs overflow-x-auto max-h-64"> + {formattedComposition.value} + </pre> + ) : loading.value ? ( + <div class="rounded-lg border bg-muted/30 p-8 text-center text-sm text-muted-foreground"> + Loading... + </div> + ) : ( + <div class="rounded-lg border bg-muted/30 p-8 text-center text-sm text-muted-foreground"> + Composition data not available. + </div> + )} + </div> +)Then use it in both tabs:
<TabsContent value="FLAT" class="mt-2"> <component :is="renderCompositionTab" /> </TabsContent> <TabsContent value="STRUCTURED" class="mt-2"> <component :is="renderCompositionTab" /> </TabsContent>Alternatively, since the content is truly identical, you could use a single template ref with v-show instead of tabs, or move the conditional rendering outside the tabs entirely.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/src/components/openehr/OpenEHRMetadataPanel.vue
🧰 Additional context used
📓 Path-based instructions (1)
web/src/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
TypeScript strict mode is enabled and implicit any types are not allowed in frontend code
Files:
web/src/components/openehr/OpenEHRMetadataPanel.vue
| <TabsContent value="STRUCTURED" class="mt-2"> | ||
| <div class="relative"> | ||
| <button | ||
| class="absolute right-2 top-2 inline-flex items-center justify-center rounded-md h-6 w-6 hover:bg-accent" | ||
| @click="copyToClipboard(formattedComposition, 'composition')" | ||
| > | ||
| <Check v-if="copiedPath === 'composition'" class="h-4 w-4 text-green-600" /> | ||
| <Copy v-else class="h-4 w-4" /> | ||
| </button> | ||
| <pre | ||
| v-if="rawComposition && !loading" | ||
| class="rounded-lg border bg-muted/30 p-3 text-xs overflow-x-auto max-h-64" | ||
| >{{ formattedComposition }}</pre> | ||
| <div v-else-if="loading" class="rounded-lg border bg-muted/30 p-8 text-center text-sm text-muted-foreground"> | ||
| Loading... | ||
| </div> | ||
| </div> | ||
| </TabsContent> |
There was a problem hiding this comment.
Add missing fallback message in STRUCTURED tab.
The STRUCTURED tab is missing the "Composition data not available" fallback that the FLAT tab has (lines 239-241). This creates an inconsistent user experience when data is unavailable.
🔎 Proposed fix
<TabsContent value="STRUCTURED" class="mt-2">
<div class="relative">
<button
class="absolute right-2 top-2 inline-flex items-center justify-center rounded-md h-6 w-6 hover:bg-accent"
@click="copyToClipboard(formattedComposition, 'composition')"
>
<Check v-if="copiedPath === 'composition'" class="h-4 w-4 text-green-600" />
<Copy v-else class="h-4 w-4" />
</button>
<pre
v-if="rawComposition && !loading"
class="rounded-lg border bg-muted/30 p-3 text-xs overflow-x-auto max-h-64"
>{{ formattedComposition }}</pre>
<div v-else-if="loading" class="rounded-lg border bg-muted/30 p-8 text-center text-sm text-muted-foreground">
Loading...
</div>
+ <div v-else class="rounded-lg border bg-muted/30 p-8 text-center text-sm text-muted-foreground">
+ Composition data not available.
+ </div>
</div>
</TabsContent>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <TabsContent value="STRUCTURED" class="mt-2"> | |
| <div class="relative"> | |
| <button | |
| class="absolute right-2 top-2 inline-flex items-center justify-center rounded-md h-6 w-6 hover:bg-accent" | |
| @click="copyToClipboard(formattedComposition, 'composition')" | |
| > | |
| <Check v-if="copiedPath === 'composition'" class="h-4 w-4 text-green-600" /> | |
| <Copy v-else class="h-4 w-4" /> | |
| </button> | |
| <pre | |
| v-if="rawComposition && !loading" | |
| class="rounded-lg border bg-muted/30 p-3 text-xs overflow-x-auto max-h-64" | |
| >{{ formattedComposition }}</pre> | |
| <div v-else-if="loading" class="rounded-lg border bg-muted/30 p-8 text-center text-sm text-muted-foreground"> | |
| Loading... | |
| </div> | |
| </div> | |
| </TabsContent> | |
| <TabsContent value="STRUCTURED" class="mt-2"> | |
| <div class="relative"> | |
| <button | |
| class="absolute right-2 top-2 inline-flex items-center justify-center rounded-md h-6 w-6 hover:bg-accent" | |
| @click="copyToClipboard(formattedComposition, 'composition')" | |
| > | |
| <Check v-if="copiedPath === 'composition'" class="h-4 w-4 text-green-600" /> | |
| <Copy v-else class="h-4 w-4" /> | |
| </button> | |
| <pre | |
| v-if="rawComposition && !loading" | |
| class="rounded-lg border bg-muted/30 p-3 text-xs overflow-x-auto max-h-64" | |
| >{{ formattedComposition }}</pre> | |
| <div v-else-if="loading" class="rounded-lg border bg-muted/30 p-8 text-center text-sm text-muted-foreground"> | |
| Loading... | |
| </div> | |
| <div v-else class="rounded-lg border bg-muted/30 p-8 text-center text-sm text-muted-foreground"> | |
| Composition data not available. | |
| </div> | |
| </div> | |
| </TabsContent> |
🤖 Prompt for AI Agents
In web/src/components/openehr/OpenEHRMetadataPanel.vue around lines 244 to 261,
the STRUCTURED TabsContent block lacks the fallback message shown in the FLAT
tab when composition data is unavailable; add a v-else (or v-else-if checking
!loading && !rawComposition) block after the existing v-if/v-else-if that
renders the same "Composition data not available" message and styling as the
FLAT tab (matching lines ~239-241) so the UI is consistent when no data is
present.
… retrieval and deletion, and enhance OpenEHR metadata panel with parallel archetype fetching and `v-model` for format switching.
…ommand for resolving code review comments.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
api/src/ehrbase/client.py (1)
148-151: Synthetic fallback response noted in prior review.The
{"status": "uploaded"}fallback was previously flagged as a synthetic response that doesn't represent actual server output. The OpenEHR API spec returns the server-assignedtemplate_idin the Location header on success. Consider returning relevant header information instead of a constructed dict.web/src/components/openehr/OpenEHRMetadataPanel.vue (3)
63-76: Race condition on rapid format switching persists.When the user quickly switches between FLAT and STRUCTURED formats, responses may arrive out of order. The previous review suggested request tracking or AbortController to discard stale responses.
🔎 Proposed fix with request tracking
+let formatRequestId = 0 + watch(activeFormat, async (format) => { if (!props.reading || !props.reading.id) return + const requestId = ++formatRequestId loading.value = true try { rawComposition.value = await store.fetchRawComposition( props.reading.id, props.patientId, format as 'FLAT' | 'STRUCTURED' ) + if (requestId !== formatRequestId) return // Discard stale response } finally { - loading.value = false + if (requestId === formatRequestId) { + loading.value = false + } } })
53-57: Missing user-facing error state.Errors are logged to console but not displayed to users. The previous review suggested adding an
errorref and displaying a message with retry option.
250-266: Missing fallback message in STRUCTURED tab.The STRUCTURED tab lacks the "Composition data not available" fallback that the FLAT tab has (line 245-247). This creates inconsistent UX when data is unavailable.
🔎 Proposed fix
<div v-else-if="loading" class="rounded-lg border bg-muted/30 p-8 text-center text-sm text-muted-foreground"> Loading... </div> + <div v-else class="rounded-lg border bg-muted/30 p-8 text-center text-sm text-muted-foreground"> + Composition data not available. + </div>
🧹 Nitpick comments (5)
api/src/ehrbase/client.py (1)
105-112: Return value afterraise_for_status()is misleading.After
raise_for_status()raises an exception for non-2xx responses, the subsequent checkresponse.status_code == 204will only evaluate when the response is successful (2xx). However, other 2xx codes like 200 would returnFalse, which may not reflect the actual deletion success.Consider simplifying to always return
Trueon success, or handle the specific status codes explicitly:🔎 Proposed fix
async def delete_composition(self, ehr_id: str, composition_uid: str) -> bool: """Delete a composition by UID. Returns True if successful.""" client = await self._get_client() response = await client.delete( f"/openehr/v1/ehr/{ehr_id}/composition/{composition_uid}" ) - response.raise_for_status() - return response.status_code == 204 + if response.status_code == 204: + return True + response.raise_for_status() # Raise for unexpected status codes + return True # Other 2xx codes also indicate successweb/src/stores/vitals.ts (1)
80-120: Consider adding loading state for metadata fetches.The
fetchRawComposition,fetchCompositionPaths, andfetchArchetypeInfomethods don't setloading.value, unlike the main CRUD operations. This is likely intentional for background fetches, but consumers won't know when these operations are in progress.If the OpenEHRMetadataPanel needs a loading indicator for these fetches, it currently manages its own
loadingref. This is acceptable but creates inconsistency with the store pattern.web/src/components/vitals/VitalSignsPanel.vue (1)
68-72: Consider usingcomputedinstead of ref + watch.The
selectedRangeLabelcould be simplified from a ref with a watcher to a computed property:🔎 Proposed simplification
-const selectedRangeLabel = ref('Last 30 days') -watch(dateRange, (val) => { - const opt = dateRangeOptions.find(o => o.value === val) - selectedRangeLabel.value = opt?.label || 'Last 30 days' -}) +const selectedRangeLabel = computed(() => { + const opt = dateRangeOptions.find(o => o.value === dateRange.value) + return opt?.label || 'Last 30 days' +})api/src/observations/service.py (2)
217-219: Silent exception handling hinders debugging.The
except Exception: passpattern swallows errors without any logging, making issues difficult to diagnose. Consider adding a warning log like elsewhere in this file:🔎 Proposed fix
except Exception: - pass - return None + logging.warning(f"Failed to get raw composition {composition_uid}: {e}") + return NoneNote: You'll need to capture the exception:
except Exception as e:
134-144: Move pagination to AQL query for better performance.AQL query fetches all matching rows from EHRBase, then pagination is applied in Python with
rows[skip : skip + limit](line 144). For patients with many vital readings, this fetches all data before discarding most of it. EHRBase AQL fully supportsOFFSETandLIMITclauses, so pagination can be pushed to the server:SELECT ... ORDER BY ... OFFSET $skip LIMIT $limitNote: EHRBase returns pagination metadata but not a total count. If the total count is needed, you'll require a separate COUNT query alongside the paginated query.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.claude/commands/resolve-code-review-comments.mdapi/pyproject.tomlapi/src/ehrbase/client.pyapi/src/medications/service.pyapi/src/observations/service.pyweb/src/components/openehr/OpenEHRMetadataPanel.vueweb/src/components/vitals/VitalSignsPanel.vueweb/src/stores/vitals.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- api/pyproject.toml
🧰 Additional context used
📓 Path-based instructions (5)
api/src/ehrbase/{client,compositions,templates,queries}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use EHRBase integration modules organized under api/src/ehrbase/ with client.py for REST API client, compositions.py for composition helpers, templates.py for template management, and queries.py for AQL query builders
Files:
api/src/ehrbase/client.py
api/src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
api/src/**/*.py: Type hints are required everywhere in Python code and enforced by mypy configuration
All functions in Python must be async
Use httpx.AsyncClient for async HTTP requests in Python; never use the synchronous requests library
Files:
api/src/ehrbase/client.pyapi/src/medications/service.pyapi/src/observations/service.py
web/src/{pages,stores,composables,types,lib}/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Frontend structure should be organized with pages/ for Vue route components, stores/ for Pinia state management, composables/ for reusable composition functions, types/ for TypeScript definitions, and lib/ for UI component library (shadcn-vue)
Files:
web/src/stores/vitals.ts
web/src/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
TypeScript strict mode is enabled and implicit any types are not allowed in frontend code
Files:
web/src/stores/vitals.tsweb/src/components/vitals/VitalSignsPanel.vueweb/src/components/openehr/OpenEHRMetadataPanel.vue
api/src/*/{router,service,repository,schemas}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Service Layer Pattern in Python API with separate modules for router.py (FastAPI route definitions), service.py (business logic layer), repository.py (Prisma database operations), and schemas.py (Pydantic models)
Files:
api/src/medications/service.pyapi/src/observations/service.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: platzhersh/open-cis PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:46:00.330Z
Learning: Applies to api/src/ehrbase/{client,compositions,templates,queries}.py : Use EHRBase integration modules organized under api/src/ehrbase/ with client.py for REST API client, compositions.py for composition helpers, templates.py for template management, and queries.py for AQL query builders
📚 Learning: 2026-01-02T20:46:00.330Z
Learnt from: CR
Repo: platzhersh/open-cis PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:46:00.330Z
Learning: Applies to api/src/ehrbase/{client,compositions,templates,queries}.py : Use EHRBase integration modules organized under api/src/ehrbase/ with client.py for REST API client, compositions.py for composition helpers, templates.py for template management, and queries.py for AQL query builders
Applied to files:
api/src/ehrbase/client.pyapi/src/observations/service.py
🧬 Code graph analysis (2)
api/src/ehrbase/client.py (2)
api/src/ehrbase/compositions.py (1)
delete_composition(50-54)api/src/ehrbase/templates.py (1)
get_template_example(31-39)
web/src/stores/vitals.ts (3)
web/src/types/vitals.ts (6)
VitalSignsReading(22-32)VitalSignsListResponse(43-46)VitalSignsCreate(34-41)RawComposition(48-52)CompositionPathsResponse(60-64)ArchetypeInfo(66-73)web/src/lib/api.ts (1)
api(40-52)api/src/observations/schemas.py (2)
VitalSignsListResponse(83-87)VitalSignsCreate(29-66)
🪛 LanguageTool
.claude/commands/resolve-code-review-comments.md
[style] ~1-~1: Consider using a different verb for a more formal wording.
Context: ...a good idea of what needs to be done to fix the issue.
(FIX_RESOLVE)
🔇 Additional comments (10)
api/src/medications/service.py (1)
3-3: LGTM! Timezone-aware datetime is a best practice improvement.The change from
datetime.utcnow()todatetime.now(UTC)is excellent. This creates a timezone-aware datetime instead of a naive one, which prevents timezone-related bugs and aligns with modern Python best practices.Also applies to: 29-29
.claude/commands/resolve-code-review-comments.md (1)
1-1: LGTM!Clear and concise guidance for resolving code review comments. The wording is appropriate for the context.
api/src/ehrbase/client.py (1)
86-103: Well-structured composition retrieval methods.The
get_compositionandget_composition_formattedmethods provide clear APIs with proper type hints and async patterns. The format parameter follows EHRBase conventions.web/src/stores/vitals.ts (2)
61-78: Query parameter handling in delete URL.Building query parameters manually in the URL string works but is less robust than using the API's params support. If
api.deletedoesn't support params, this is fine, but consider URL encoding edge cases forcompositionUidvalues that may contain special characters like::.The URL encoding via
encodeURIComponentis correctly applied.
1-18: Well-structured Pinia store with proper TypeScript typing.The store follows Vue 3 composition API patterns with proper reactive state management. Type imports from
@/typesensure type safety across the application.web/src/components/vitals/VitalSignsPanel.vue (1)
1-73: Well-implemented vital signs panel with proper reactivity.The component correctly handles:
- Initial data load via
watchwithimmediate: true- Date range changes triggering reload
- Refresh after recording new vitals (past issue addressed)
- Proper separation of concerns with child components
web/src/components/openehr/OpenEHRMetadataPanel.vue (1)
37-52: Good improvement: Parallel archetype fetching.Using
Promise.allto fetch archetype info concurrently is a performance improvement over the previous sequential approach. The filtering of already-cached archetypes avoids redundant requests.api/src/observations/service.py (3)
189-219: Past issues addressed: Now uses public client methods.The service correctly uses
ehrbase_client.delete_compositionandehrbase_client.get_composition_formattedinstead of accessing the private_get_client()method. This follows proper encapsulation.
221-265: Well-structured FLAT composition builder.The
_build_flat_compositionmethod correctly assembles the openEHR FLAT format with proper terminology codes and conditional inclusion of blood pressure and pulse data.
267-321: Comprehensive openEHR metadata generation.The
_build_openehr_metadatamethod provides excellent transparency by mapping UI fields to archetype paths. This enables the frontend to show users exactly how their data is stored in openEHR format.
…data fetching, and remove unused template example endpoint.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
web/src/components/openehr/OpenEHRMetadataPanel.vue (1)
283-300: Add missing fallback message in STRUCTURED tab.The STRUCTURED tab lacks the "Composition data not available" fallback that the FLAT tab has (lines 278-280), creating inconsistent UX when data is unavailable.
🔎 Proposed fix
<TabsContent value="STRUCTURED" class="mt-2"> <div class="relative"> <button class="absolute right-2 top-2 inline-flex items-center justify-center rounded-md h-6 w-6 hover:bg-accent" @click="copyToClipboard(formattedComposition, 'composition')" > <Check v-if="copiedPath === 'composition'" class="h-4 w-4 text-green-600" /> <Copy v-else class="h-4 w-4" /> </button> <pre v-if="rawComposition && !loading" class="rounded-lg border bg-muted/30 p-3 text-xs overflow-x-auto max-h-64" >{{ formattedComposition }}</pre> <div v-else-if="loading" class="rounded-lg border bg-muted/30 p-8 text-center text-sm text-muted-foreground"> Loading... </div> + <div v-else class="rounded-lg border bg-muted/30 p-8 text-center text-sm text-muted-foreground"> + Composition data not available. + </div> </div> </TabsContent>
🧹 Nitpick comments (4)
api/src/ehrbase/templates.py (2)
31-38: Consider filtering empty template IDs.Line 35 could include empty strings if a template dict is missing the
template_idfield. While this won't cause false positives in the current usage, filtering them out would make the returned list cleaner.🔎 Optional refactor to filter empty strings
async def get_registered_template_ids() -> list[str]: """Get list of template IDs registered in EHRBase.""" try: templates = await list_templates() - return [t.get("template_id", "") for t in templates] + return [tid for t in templates if (tid := t.get("template_id"))] except Exception as e: logger.warning(f"Failed to list EHRBase templates: {e}") return []
59-121: LGTM! Comprehensive template registration with proper async I/O.The function correctly addresses previous issues by using aiofiles for async file reading and staying within line length limits. The logic is sound: checking existing registrations, uploading missing templates, and providing clear summary logging.
🔎 Optional: Explicit mode parameter for clarity
Line 105 relies on the default mode='r' for text reading. Consider making it explicit:
- async with aiofiles.open(template_file, encoding="utf-8") as f: + async with aiofiles.open(template_file, mode="r", encoding="utf-8") as f: template_content = await f.read()web/src/components/openehr/OpenEHRMetadataPanel.vue (2)
24-24: Consider stricter typing foractiveFormat.The
activeFormatref is currently inferred asRef<string>, but only'FLAT'and'STRUCTURED'are valid values. Adding an explicit type annotation would improve type safety and prevent potential bugs.🔎 Proposed fix
-const activeFormat = ref('FLAT') +const activeFormat = ref<'FLAT' | 'STRUCTURED'>('FLAT')This eliminates the need for the type assertion on line 40.
52-64: UsePromise.allSettledfor graceful archetype loading.Currently, if fetching info for any single archetype fails, the entire
loadDataoperation fails and users see an error banner instead of the composition data. UsingPromise.allSettledwould allow the component to display the composition and successfully loaded archetype info even if some archetype fetches fail.🔎 Proposed fix for resilient archetype fetching
if (archetypesToFetch.length > 0) { - const archetypeResults = await Promise.all( - archetypesToFetch.map(archetypeId => store.fetchArchetypeInfo(archetypeId)) - ) + const archetypeResults = await Promise.allSettled( + archetypesToFetch.map(archetypeId => + store.fetchArchetypeInfo(archetypeId).then(info => ({ archetypeId, info })) + ) + ) // Only update if this is still the latest request if (currentRequestId === requestId) { - archetypesToFetch.forEach((archetypeId, index) => { - const info = archetypeResults[index] - if (info) { - archetypeInfos.value.set(archetypeId, info) - } - }) + archetypeResults.forEach(result => { + if (result.status === 'fulfilled' && result.value.info) { + archetypeInfos.value.set(result.value.archetypeId, result.value.info) + } + }) } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/src/ehrbase/templates.pyweb/src/components/openehr/OpenEHRMetadataPanel.vueweb/src/components/vitals/RecordVitalsDialog.vue
🧰 Additional context used
📓 Path-based instructions (3)
web/src/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
TypeScript strict mode is enabled and implicit any types are not allowed in frontend code
Files:
web/src/components/vitals/RecordVitalsDialog.vueweb/src/components/openehr/OpenEHRMetadataPanel.vue
api/src/ehrbase/{client,compositions,templates,queries}.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use EHRBase integration modules organized under api/src/ehrbase/ with client.py for REST API client, compositions.py for composition helpers, templates.py for template management, and queries.py for AQL query builders
Files:
api/src/ehrbase/templates.py
api/src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
api/src/**/*.py: Type hints are required everywhere in Python code and enforced by mypy configuration
All functions in Python must be async
Use httpx.AsyncClient for async HTTP requests in Python; never use the synchronous requests library
Files:
api/src/ehrbase/templates.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: platzhersh/open-cis PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:46:00.330Z
Learning: Applies to api/src/ehrbase/{client,compositions,templates,queries}.py : Use EHRBase integration modules organized under api/src/ehrbase/ with client.py for REST API client, compositions.py for composition helpers, templates.py for template management, and queries.py for AQL query builders
📚 Learning: 2026-01-02T20:46:00.330Z
Learnt from: CR
Repo: platzhersh/open-cis PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:46:00.330Z
Learning: Applies to api/src/ehrbase/{client,compositions,templates,queries}.py : Use EHRBase integration modules organized under api/src/ehrbase/ with client.py for REST API client, compositions.py for composition helpers, templates.py for template management, and queries.py for AQL query builders
Applied to files:
api/src/ehrbase/templates.py
📚 Learning: 2026-01-02T20:46:00.330Z
Learnt from: CR
Repo: platzhersh/open-cis PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:46:00.330Z
Learning: Applies to api/src/**/*.py : Type hints are required everywhere in Python code and enforced by mypy configuration
Applied to files:
api/src/ehrbase/templates.py
🔇 Additional comments (9)
api/src/ehrbase/templates.py (3)
3-18: LGTM! Imports and module setup are well-structured.The imports correctly support async operations (aiofiles, httpx) and the REQUIRED_TEMPLATES constant provides clear configuration. Previous concerns about missing type stubs for aiofiles have been addressed.
21-28: LGTM! Clean wrapper functions.Both functions properly delegate to the EHR client with correct async signatures and type hints.
41-56: Excellent fix for HTTP 409 handling!The function now properly inspects the HTTP status code using
httpx.HTTPStatusErrorinstead of fragile string matching. The exception handling is robust and the logging is clear.web/src/components/openehr/OpenEHRMetadataPanel.vue (1)
29-77: Excellent race condition handling!The implementation properly addresses the previous race condition concerns using a
requestIdpattern. Both rapid panel switching and format changes are now handled correctly—stale responses are discarded and only the latest request updates the UI state. The error handling with user feedback and retry functionality is also well implemented.web/src/components/vitals/RecordVitalsDialog.vue (5)
1-21: LGTM: Clean component setup.The imports, props, emits, and store setup follow Vue 3 and TypeScript best practices. All types are explicit, meeting the strict mode requirements.
79-85: LGTM: Robust empty string handling.The
normalizeNumberhelper correctly addresses the v-model.number quirk where cleared inputs produce empty strings. The function defensively handles all edge cases (empty string, undefined, null, numeric strings, NaN) and ensures consistent null values for empty inputs.
137-164: LGTM: Robust submission with proper normalization.The submission handler correctly:
- Uses
normalizeNumberfor all numeric inputs, ensuring empty strings becomenullinstead of being sent to the backend- Implements comprehensive error handling with try-catch-finally
- Validates
canSubmitbefore proceeding- Emits the created reading and closes the dialog on success
219-248: LGTM: Good datetime validation and UX.The datetime input correctly:
- Uses
datetime-localtype with required attribute- Provides a convenient "Now" button for quick entry
- Displays validation errors (line 245-247) when the time is in the future
- Triggers validation on change (line 235)
The staleness issue with
maxDateTimehas been flagged separately.
250-308: LGTM: Well-structured vital signs inputs.The blood pressure and pulse inputs are well-designed:
- Reasonable HTML5 validation ranges that cover clinical edge cases
- Clear unit labels (mmHg, bpm)
- Helpful validation message ensuring BP values are provided together
- Good use of v-model.number with the
normalizeNumberhelper handling empty inputs
| const maxDateTime = computed(() => { | ||
| const now = new Date() | ||
| const pad = (n: number) => n.toString().padStart(2, '0') | ||
| return `${now.getFullYear()}-${pad(now.getMonth() + 1)}-${pad(now.getDate())}T${pad(now.getHours())}:${pad(now.getMinutes())}` | ||
| }) |
There was a problem hiding this comment.
maxDateTime becomes stale over time.
The maxDateTime computed property has no reactive dependencies, so it's calculated once when first accessed and cached indefinitely. If a user opens the dialog at 10:00 AM and clicks "Now" five minutes later, setNow() sets recordedAt to 10:05 AM, but the HTML5 max attribute remains "10:00 AM", causing the browser to block form submission.
🔎 Proposed fix: Remove max attribute, rely on JS validation
The isDateTimeValid computed (line 105-110) already prevents future dates using the current time at validation, so the max attribute is redundant. Removing it avoids the staleness issue:
-const maxDateTime = computed(() => {
- const now = new Date()
- const pad = (n: number) => n.toString().padStart(2, '0')
- return `${now.getFullYear()}-${pad(now.getMonth() + 1)}-${pad(now.getDate())}T${pad(now.getHours())}:${pad(now.getMinutes())}`
-})
-Then remove the :max binding in the template:
<input
id="recorded-at"
v-model="recordedAt"
type="datetime-local"
required
- :max="maxDateTime"
:class="[The existing JS validation (isDateTimeValid + validateDateTime) already enforces the business rule correctly.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In web/src/components/vitals/RecordVitalsDialog.vue around lines 112-116, the
computed maxDateTime has no reactive dependencies and becomes stale; remove the
maxDateTime computed and the :max binding on the datetime-local input in the
template so the browser-side max attribute is not stale, and rely on the
existing isDateTimeValid/validateDateTime logic to prevent future dates; ensure
any references to maxDateTime are removed and tests/validation still pass.
2 participants
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.