Project: doctolib-doctolib · Branch: .invalid · Mode: · Model: claude-sonnet-4-6 Stats: 1 user messages, 2 tool calls, tools used: Read. Conversation Turn 1 — User Please do a full review of the NHR observability implementation plan at: /Users/USER/Library/CloudStorage//My Drive/ai-artifacts/nhr-document-list/observability/implementation-plan.md The plan has been significantly redesigned since the last review. Key changes: 1. NhrRequestDetails and NhrResponseDetails are now discriminated union types keyed on action 2. Two additional mapped types: NhrRequestActionFields and NhrResponseActionFields for a generic helper 3. NhrAdapterWithObservability now has a private withObservability helper that centralises all boilerplate — each public method is now 10-15 lines 4. Architecture and Testing sections updated to reflect the new design Review for: 1. Is the motivation for discriminated union types vs flat types clearly explained? 2. Is the withObservability helper and the reason for two type shapes (union + mapped) explained clearly enough for the implementing engineer? 3. Are the code samples correct and readable? 4. Is the testing strategy updated correctly to reflect that withObservability invariants only need testing once? 5. Any remaining gaps, ambiguities, or documentation issues? Be specific — provide actionable feedback for each issue found. If the document is ready, say so clearly. Turn 1 — Assistant I'll read the implementation plan document thoroughly before reviewing it. Tools used: - Read: /Users/USER/Library/CloudStorage//My Drive/ai-artifacts/nhr-document-list/observability/implementation-plan.md Tool results: → result (ok): 1 # NHR Adapter Observability — Implementation Plan 2 3 ## Context 4 5 The National Health Records (NHR) feature is built on a 3-layer architecture: 6 7 | Layer | Package | Ownership | 8 |---|---|---| 9 | 1 — Local (France DMP) | @doctolib/dmp | @doctolib/moose — read-only | 10 | 2 — Adapter | @doctolib/national-health-records | @doctolib/fire | 11 | 3 — Generic interface | @doctolib/tpf-national-health-records | @doctolib/fire | 12 13 The adapter layer (@doctolib/national-health- …(truncated, 30276 more chars) I've read the full document. Let me also check the actual DmpNhrAdapter.ts to verify whether the code samples accurately reflect the real types and method signatures before giving feedback. Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts Tool results: → result (ok): 1 DmpApi, 2 DmpAuthor, 3 DmpDocument, 4 DmpDocumentContent, 5 SearchProps, 6 SearchResult as DmpSearchResult, 7 } from '@doctolib/dmp' 8 import { DmpApiError, DOCUMENT_STATUS, DOCUMENT_VISIBILITY, getPatientInsForDMP } from '@doctolib/dmp' 9 import { isUnstructured } from '@doctolib/cda/types/UnstructuredBody' 10 import { isStructured } from '@doctolib/cda/types/StructuredBody' 11 import type { Patient } from 'monolith_DEPRECATED/types/doctor/api/Patient' 12 import type { 13 NhrAdapt …(truncated, 9700 more chars) Now I have everything I need. Here is the full review. --- Review: NHR Adapter Observability Implementation Plan Overall assessment: The plan is well-structured and the redesign is a genuine improvement. The withObservability centralisation is a strong architectural decision and the type system explanation is mostly clear. There are five issues that need fixing before this is implementation-ready, and several smaller ones worth addressing. --- Issue 1 — Discriminated union motivation is correct but the explanation conflates two separate problems Section: "Why discriminated union types" (nhrMonitoring.ts prose block) Problem: The explanation says discriminated unions prevent passing documents_count to an update_document_metadata event "without a compiler error." This is accurate, but the plan then immediately introduces NhrRequestActionFields / NhrResponseActionFields as a second type shape for withObservability. An implementing engineer will reasonably ask: "If the discriminated union already prevents invalid field combinations, why do I also need the mapped types? Are the two shapes redundant? Which one is the source of truth?" The answer — that TypeScript cannot narrow a generic TAction extends NhrAdapterAction into a union branch at a spread site, so index access on a mapped type is required at the call site — is buried in the NhrAdapterWithObservability.ts prose and repeated in the withObservability comment. This leaves the reader assembling the full picture from three separate locations. Fix: Add a single short paragraph immediately after the discriminated union motivation that says: "The discriminated union types (NhrRequestDetails / NhrResponseDetails) are the public API for sendNhrRequest / sendNhrResponse. Inside withObservability, the helper is generic over TAction — TypeScript cannot collapse a generic into a union branch at the spread site, so a second pair of mapped types (NhrRequestActionFields / NhrResponseActionFields) is used to constrain what each caller passes. A single as NhrRequestDetails cast is isolated inside withObservability; all callers are fully type-checked without casts." This makes the two-shape design self-contained in one place. --- Issue 2 — NhrResponseActionFields omits error_code for get_documents_content Section: NhrResponseActionFields type definition in the code sample Problem: The get_documents_content branch in NhrResponseActionFields (lines 264–268 in the plan) includes only status, requested_count, and returned_count. It does not include error_code or error_message. However, looking at DmpNhrAdapter.getDocumentsContent, the method throws on error (e.g., throw new Error('Patient is mandatory for DMP')). The catch branch in withObservability sets error_message directly on the response object (not via buildSuccessFields), so error_message is not needed in the mapped type for the success path. But error_code is also absent from the search_documents mapped-type entry in NhrResponseActionFields (lines 253–262), even though the searchDocuments method's buildSuccessFields closure explicitly populates it: error_code: result.status === 'error' ? result.code : undefined, error_message: result.status === 'error' ? result.message : undefined, These fields are part of NhrResponseActionFields['search_documents'], yet the mapped type definition in the plan does not include them. TypeScript will reject this at compile time because the buildSuccessFields return value is checked against NhrResponseActionFields[TAction]. Fix: Add error_code?: string and error_message?: string to the search_documents entry in NhrResponseActionFields. The get_documents_content entry is correct as written (errors go through the catch branch, not buildSuccessFields). Also confirm update_document_metadata is correct — the plan says error_code is "not emitted for update_document_metadata", which is consistent with the mapped type definition. --- Issue 3 — NhrDocumentMetadataUpdatePayloadResult does not have a propertyName field typed as string | undefined; the filter call in propertyNames extraction is unnecessary noise and the type claim may be wrong Section: updateDocumentMetadata method, lines 419–421 Problem: The plan writes: const propertyNames = [ ...new Set(payloads.map((p) => p.propertyName).filter((n): n is string => n !== undefined)), ] Looking at the actual DmpNhrAdapter.ts (lines 221–222), payload.propertyName is destructured and used directly without an undefined guard. The NhrDocumentMetadataUpdateProps type comes from @doctolib/tpf-national-health-records, which this plan does not show. If propertyName is typed as string (not string | undefined), the filter with the type predicate is dead code that adds noise. If it is typed as string | undefined, the comment in the plan should explain why a payload could have no property name at all — that would be a meaningful domain observation. Action needed: Verify the type of NhrDocumentMetadataUpdatePayload.propertyName in @doctolib/tpf-national-health-records. If it is string, remove the .filter() and use a plain .map(). If it is string | undefined, add a one-sentence note in the plan explaining when that can happen. --- Issue 4 — The wiring change removes the named variable but the comment about useMemo no longer matches the code shown Section: "Wiring in src/hooks/useDmpNhr.ts" Problem: The plan says "The NhrAdapterWithObservability instance is created inside useMemo alongside DmpNhrAdapter — it is a pure wrapper with no state of its own, so it does not require separate memoization." This is correct. But the "To this" code collapses everything into one inline expression: const nhrService = useMemo( () => createNhrService('fr', new NhrAdapterWithObservability(new DmpNhrAdapter(dmpApi, facets), 'dmp')), [dmpApi, facets] ) This is functionally correct, but an engineer reading the original file (which has a separate const adapter = ... line) may wonder whether losing the named intermediate variable is intentional or an oversight. The plan does not say "the named adapter variable is removed intentionally." This is a minor ambiguity but worth one sentence: "The intermediate adapter variable is removed — it has no other references." --- Issue 5 — Testing section is missing a test case for 'partial success' status in updateDocumentMetadata Section: "Per-method tests (action-specific fields only)" Problem: DmpNhrAdapter.buildUpdateResult can return three statuses: 'success', 'error', and 'partial success'. The 'partial success' case (mixed success and error payloads) is a meaningful edge case that directly affects success_count, error_count, and the top-level status field. The testing section mentions verifying success_count, error_count, and skipped_count but does not call out the 'partial success' scenario as a required test case. An engineer following only this plan will likely miss it. Fix: Add a bullet under updateDocumentMetadata: "Verify that a mix of success and error payloads produces status: 'partial success' with correct success_count and error_count." --- Smaller Issues (low priority, but worth fixing) NhrRequestDetails uses optional fields for request-time data that should be required In NhrRequestDetails, the get_documents_content and update_document_metadata branches use optional fields (requested_count?, payloads_count?, property_names?). But withObservability` always provides these values at the call site (lines 405, 425). Making them optional weakens the type contract for no benefit — if the value is always available, require it. This is a type-correctness concern, not just style. …(truncated, 2874 more chars)