Project: doctolib-doctolib · Branch: .invalid · Mode: ` · **Model:**claude-sonnet-4-6`
Stats: 1 user messages, 12 tool calls, tools used: Bash, Read.
Conversation
Turn 1 — User
Re-review PR #242170 in the monolith repo at /Users/USER/workspace/doctolib/doctolib (branch fhir-2245_observability-search-documents) for UpFront architecture compliance. Focus on whether any critical issues remain.
Files to review:
1. packages/@doctolib/national-health-records/src/business/adapters/nhrMonitoring.ts
2. packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.ts
3. packages/@doctolib/national-health-records/src/hooks/useDmpNhr.ts
Important context for evaluating the "src/business must be pure" concern raised in the previous review:
- DmpNhrAdapter.ts (at the same path src/business/adapters/) already makes real network calls via this.dmpApi.search(), this.dmpApi.getDocumentsContentByUniqueId(), etc.
- So src/business/ in THIS package already contains side-effectful code by established convention
- nhrMonitoring.ts is collocated with DmpNhrAdapter.ts following the single-consumer utility precedent (parseHl7DateToIso.ts also lives in src/business/adapters/)
Please read DmpNhrAdapter.ts to verify this before flagging the file placement as a critical issue. Report only issues that remain blockers after verifying this context. Use file:line references.
Turn 1 — Assistant
I'll review the files and context before providing feedback.
Tools used:
Bash:git log --oneline -5 origin/fhir-2245_observability-search-documents 2>/dev/null || git log --oneline -5 2>/dev/null
Tool results:
→ result (ok): 81e3037ead18 test(FHIR-2245): add missing test coverage for NHR adapter observability 2d9b1ae4f932 feat(FHIR-2245): add Datadog observability to NHR adapter searchDocuments fb3b3fc5e3f4 chore(DITTO-4484): warn when documents archive has no file entries (#241994) 9480cea7a4d1 chore(MDP-3565): add logs for debugging (#242165) 5370763ed3e6 feat(MODUS-3515): surface secondary-link managing accounts in patient file (#241594)
Tools used:
Read:/Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts
Tool results:
→ result (ok): 1 import type { 2 DmpApi, 3 DmpAuthor, 4 DmpDocument, 5 DmpDocumentContent, 6 SearchProps, 7 SearchResult as DmpSearchResult, 8 } from '@doctolib/dmp' 9 import { DmpApiError, DOCUMENT_STATUS, DOCUMENT_VISIBILITY, getPatientInsForDMP } from '@doctolib/dmp' 10 import { isUnstructured } from '@doctolib/cda/types/UnstructuredBody' 11 import { isStructured } from '@doctolib/cda/types/StructuredBody' 12 import type { Patient } from 'monolith_DEPRECATED/types/doctor/api/Patient' 13 import t …(truncated, 9720 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/nhrMonitoring.ts
Tool results:
→ result (ok): 1 import { sendMonitoringEvent } from 'monolith_DEPRECATED/services/monitoring'
2 import type { NhrResultStatus } from '@doctolib/tpf-national-health-records'
3
4 export type NhrAdapterName = 'dmp'
5 // Extend when adding adapters: | 'germany-kim' | 'italy-fse'
6
7 export type NhrAdapterAction = 'search_documents'
8 // Extend in follow-up increments: | 'get_documents_content' | 'update_document_metadata'
9
10 // ── Discriminated union types ──
11 // Used by sendNhrRequest / sendNhrResponse —
…(truncated, 2534 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.ts
Tool results:
→ result (ok): 1 import type { 2 NhrAdapter, 3 NhrDocumentSearchProps, 4 NhrDocumentSearchResult, 5 NhrGetDocumentsContentProps, 6 NhrDocumentContentResult, 7 NhrDocumentMetadataUpdateProps, 8 NhrDocumentMetadataUpdateResult, 9 } from '@doctolib/tpf-national-health-records' 10 import { sendNhrRequest, sendNhrResponse } from './nhrMonitoring' 11 import type { 12 NhrAdapterName, 13 NhrAdapterAction, 14 NhrRequestDetails, 15 NhrResponseDetails, 16 NhrRequestActionFields, 17 NhrResponseAc …(truncated, 3855 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/hooks/useDmpNhr.ts
Tool results:
→ result (ok): 1 import { useMemo } from 'react' 2 import type { Patient } from 'monolith_DEPRECATED/types/doctor/api/Patient' 3 import { DmpApiImpl as DmpApi, useDmpSetUp } from '@doctolib/dmp' 4 import { createNhrService } from '@doctolib/tpf-national-health-records' 5 import { useDocumentCategorizationFacetsIndex } from 'engines/document_categorization/queries/document_categorization/facets' 6 import { DmpNhrAdapter } from '../business/adapters/DmpNhrAdapter' 7 import { NhrAdapterWithObservability } from '. …(truncated, 917 more chars)
Now let me check the package structure and existing patterns for completeness.
Tools used:
Bash:find /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters -type f | sort
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.test.ts /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.test.ts /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/NhrAd …(truncated, 531 more chars)
Tools used:
Bash:find /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src -type f | sort
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.test.ts /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.test.ts /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/NhrAd …(truncated, 6145 more chars)
Tools used:
Bash:git diff origin/main...origin/fhir-2245_observability-search-documents -- 'packages/@doctolib/national-health-records/src/business/adapters/nhrMonitoring.ts' 'packages/@doctolib/national-health-record …(truncated, 147 more chars)
Tool results:
→ result (ok): (Bash completed with no output)
Tools used:
Bash:git diff main...HEAD -- 'packages/@doctolib/national-health-records/src/business/adapters/nhrMonitoring.ts' 'packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservabilit …(truncated, 97 more chars)
Tool results:
→ result (ok): (Bash completed with no output)
Tools used:
Bash:git log --oneline -3 && git status && git diff HEAD~2 -- 'packages/@doctolib/national-health-records/src/business/adapters/nhrMonitoring.ts' 'packages/@doctolib/national-health-records/src/business/ad …(truncated, 130 more chars)
Tool results:
→ result (ok): 81e3037ead18 test(FHIR-2245): add missing test coverage for NHR adapter observability 2d9b1ae4f932 feat(FHIR-2245): add Datadog observability to NHR adapter searchDocuments fb3b3fc5e3f4 chore(DITTO-4484): warn when documents archive has no file entries (#241994) On branch fhir-2245_observability-search-documents Your branch is up to date with 'origin/fhir-2245_observability-search-documents'.
It took 2.83 seconds to enumerate untracked files. See 'git help status' for information on how to imp …(truncated, 9321 more chars)
I now have everything I need to perform a thorough review. Let me check one more detail about the NhrResponseDetails cast issue.
Tools used:
Read:/Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.test.ts
Tool results:
→ result (ok): 1 import { sendMonitoringEvent } from 'monolith_DEPRECATED/services/monitoring'
2 import type { NhrAdapter, NhrDocumentSearchProps } from '@doctolib/tpf-national-health-records'
3 import { NhrAdapterWithObservability } from './NhrAdapterWithObservability'
4
5 jest.mock('monolith_DEPRECATED/services/monitoring', () => ({
6 sendMonitoringEvent: jest.fn().mockResolvedValue(undefined),
7 }))
8
9 const mockSendMonitoringEvent = sendMonitoringEvent as jest.MockedFunction<typeof sendMonitoringEvent
…(truncated, 11643 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/nhrMonitoring.test.ts
Tool results:
→ result (ok): 1 import { sendMonitoringEvent } from 'monolith_DEPRECATED/services/monitoring'
2 import { sendNhrRequest, sendNhrResponse } from './nhrMonitoring'
3
4 jest.mock('monolith_DEPRECATED/services/monitoring', () => ({
5 sendMonitoringEvent: jest.fn().mockResolvedValue(undefined),
6 }))
7
8 const mockSendMonitoringEvent = sendMonitoringEvent as jest.MockedFunction<typeof sendMonitoringEvent>
9
10 beforeEach(() => {
11 jest.clearAllMocks()
12 mockSendMonitoringEvent.mockResolvedValue(undefine
…(truncated, 2117 more chars)
I now have a complete picture. Here is the architecture review.
Architecture Review: PR #242170 — NHR Adapter Observability
Files reviewed:
- /packages/@doctolib/national-health-records/src/business/adapters/nhrMonitoring.ts
- /packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.ts
- /packages/@doctolib/national-health-records/src/hooks/useDmpNhr.ts
Critical Issues (must fix)
None. The one issue that would have been flagged — placement of side-effectful code under src/business/ — does not apply here. DmpNhrAdapter.ts at the same path already performs real network I/O (this.dmpApi.search(), this.dmpApi.getDocumentsContentByUniqueId(), etc.), establishing that src/business/adapters/ in this package is intentionally the home for adapter-layer code that crosses the purity boundary. nhrMonitoring.ts has a single consumer (NhrAdapterWithObservability.ts) and sits alongside parseHl7DateToIso.ts, which follows the same single-consumer colocation precedent. File placement is consistent with the established pattern.
Warnings (should fix)
1. nhrMonitoring.ts:31-42 — NhrRequestDetails and NhrResponseDetails are not discriminated unions, despite the comment claiming they are.
The comment at line 11 says "Discriminated union types" but both NhrRequestDetails (line 31) and NhrResponseDetails (line 33) are single intersection types, not unions. The discriminant (action) is fixed to 'search_documents' in both. This means when future actions are added (the extend comments at lines 5 and 8 anticipate this), the current design will require revisiting these types entirely rather than just extending the union.
The correct pattern for a discriminated union would be:
// When a second action exists:
export type NhrRequestDetails =
| (NhrRequestBase & { action: 'search_documents' })
| (NhrRequestBase & { action: 'get_documents_content' })
For now with a single action this is not a runtime bug, but the comment misrepresents the type and will mislead whoever adds the next action. Either remove the "Discriminated union" comment and replace it with "Single-action type, extend to union when second action is added", or restructure the types now.
2. NhrAdapterWithObservability.ts:51,63,75 — Three as casts against NhrRequestDetails / NhrResponseDetails that are wider than necessary.
The casts are documented as intentional (comment at line 32), but they silence TypeScript's ability to catch field mismatches when the mapped types and the detail types diverge in future. This is a direct consequence of the type design in Warning 1: if NhrRequestActionFields[TAction] were structurally unified with NhrRequestDetails, the spread would type-check without a cast. Not a blocker for this PR, but worth noting because each new action added to the mapped types will require verifying that the casts remain sound by hand.
Suggestions (consider improving)
1. useDmpNhr.ts:22 — Nested new expression inside useMemo reduces readability without architectural benefit.
The previous shape of this code used a named intermediate variable (const adapter = new DmpNhrAdapter(...)). The new form nests two constructor calls inline:
createNhrService('fr', new NhrAdapterWithObservability(new DmpNhrAdapter(dmpApi, facets), 'dmp'))
This is not an architecture violation, but it is harder to scan than:
const adapter = new NhrAdapterWithObservability(new DmpNhrAdapter(dmpApi, facets), 'dmp')
return createNhrService('fr', adapter)
The previous code used this named-variable style. The change drops it for no functional reason.
2. nhrMonitoring.ts:17 — PII comment does not reference a tracking mechanism.
Line 17 contains: patient_ins?: string // raw INS — PII, requires DPO sign-off before production. If this field is already present in the emitted monitoring payload without DPO sign-off, shipping to production with this field populated is a data governance risk regardless of whether it's just a comment. Either the field should be excluded from the payload entirely until sign-off is obtained, or the comment should reference a ticket tracking the DPO approval. As written it looks like a to-do that could be overlooked.
3. nhrMonitoring.ts — File has no default export justification, which is correct, but the file name uses lowercase (nhrMonitoring.ts) while it exports both types and functions.
Per naming conventions, multi-export files use kebab-case. nhrMonitoring.ts uses camelCase. The correct name would be nhr-monitoring.ts. All internal imports in NhrAdapterWithObservability.ts and both test files would need updating. This is a naming convention violation, not a structural one, so it is a warning-level item being listed here as a suggestion given that the rest of the codebase does not appear to enforce this strictly for all utility files (e.g. parseHl7DateToIso.ts is also camelCase). Bring it in line with the standard if the team wants consistency enforced at this layer.
Summary
No critical architecture violations remain after verifying the src/business/adapters/ precedent established by DmpNhrAdapter.ts. The PR introduces a well-structured decorator pattern with solid test coverage. The items above are worth addressing before merge, with Warning 1 (the misleading "discriminated union" comment/type design) being the most actionable given it will affect whoever extends the monitoring types next.