Project: doctolib-doctolib · Branch: .invalid · Mode: ` · **Model:**claude-sonnet-4-6`
Stats: 1 user messages, 8 tool calls, tools used: Read, Bash.
Conversation
Turn 1 — User
Please do a full review of the NHR observability implementation plan at: /Users/USER/Library/CloudStorage/<REDACTED>/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 added: NhrRequestActionFields and NhrResponseActionFields (indexed by NhrAdapterAction) for use inside a generic helper
3. NhrAdapterWithObservability now has a private withObservability<TResult, TAction> helper that centralises trace ID, timing, request/response events, try/catch, and re-throw — each public method delegates to it
4. The as NhrRequestDetails / as NhrResponseDetails casts are now isolated inside withObservability
For context, read these source files before reviewing:
- packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts
- packages/@doctolib/national-health-records/src/hooks/useDmpNhr.ts
- packages/@doctolib/national-health-records/package.json
Review the plan for:
1. TypeScript correctness of the union + mapped type design
2. Whether the withObservability generic is sound and the casts are correctly justified
3. UpFront architecture compliance (file placement, naming, export decisions)
4. Any remaining anti-patterns, issues, or improvements
Be specific — provide a structured list of issues found (if any) with recommendations. If the plan is implementation-ready, say so clearly.
Turn 1 — Assistant
I'll read all the relevant files before reviewing the implementation plan.
Tools used:
Read:/Users/USER/Library/CloudStorage/<REDACTED>/My Drive/ai-artifacts/nhr-document-list/observability/implementation-plan.md
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)
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)
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 type { UseNhrOperations } from './useNa …(truncated, 812 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/package.json
Tool results:
→ result (ok): 1 {
2 "name": "@doctolib/national-health-records",
3 "version": "1.0.0",
4 "publishConfig": {
5 "registry": "https://doctolib-580698825394.d.codeartifact.eu-west-1.amazonaws.com/npm/npm-internal/"
6 },
7 "repository": {
8 "type": "git",
9 "url": "git+https://github.com/doctolib/doctolib.git"
10 },
11 "keywords": [
12 "upfront",
13 "modular frontend",
14 "in-monolith-package"
15 ],
16 "engines": {
17 "node": ">= 24.8.0",
18 "npm": ">= 11.6.0"
19
…(truncated, 1260 more chars)
Now let me check the existing package structure, the src/index.ts, and the existing helpers directory to understand the full picture.
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/parseHl7DateToIso.test.ts /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/parseHl7DateToI …(truncated, 5619 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/index.ts
Tool results:
→ result (ok): 1 export { useNationalHealthRecords, type UseNhrOperations } from './hooks/useNationalHealthRecords' 2 export { useDmpNhr } from './hooks/useDmpNhr' 3 export { useNhrDocumentListVariant } from './variants/useNhrDocumentListVariant' 4 export type { DocumentSearchOptions, NhrDocumentListVariant } from './variants/types' 5 export { actionsBuilder, type NhrActionsBuilder } from './business/builders/actionsBuilder' 6 export { useGetDocumentsContent } from './hooks/useGetDocumentsContent' 7 export { u …(truncated, 650 more chars)
Now I have everything I need. Let me also check the ti_connect reference implementation briefly to compare patterns.
Tools used:
Bash:find /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/ti_connect/src/utils -name "callTiConnectApi.ts" 2>/dev/null | head -5
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/ti_connect/src/utils/callTiConnectApi.ts
Tools used:
Read:/Users/USER/workspace/doctolib/doctolib/packages/@doctolib/ti_connect/src/utils/callTiConnectApi.ts
Tool results:
→ result (ok): 1 import type { TiConnectorRequest } from '@doctolib/ti_connect/src/types/connector'
2
3 import { invokeElectronEvent } from '@doctolib/desktop/events'
4 import { TiConnectAction } from '@doctolib/ti_connect/src/types/ti-connect-action'
5 import { TiEvent } from '@doctolib/ti_connect/src/types/ti-event'
6 import { v4 as uuidv4 } from 'uuid'
7 import { sendMonitoringEvent } from 'monolith_DEPRECATED/services/monitoring'
8
9 interface TiConnectResponsePayload {
10 success: boolean
11 /**
12
…(truncated, 3554 more chars)
I now have everything needed for a thorough review. Here is the full analysis.
Architecture Review: NHR Adapter Observability Implementation Plan
Critical Issues (must fix)
1. src/helpers/ directory does not exist — nhrMonitoring.ts placement needs verification against actual package structure
The package has no src/helpers/ directory. The existing structure uses src/business/utils/, src/business/adapters/, src/business/builders/, src/business/mappers/, and src/business/constants/. There is no precedent for a top-level src/helpers/ layer in this package.
The UpFront taxonomy does define src/helpers/ as valid for stateless utilities, so the plan is architecturally correct on paper. However, nhrMonitoring.ts is not a general-purpose utility — it is exclusively consumed by NhrAdapterWithObservability.ts and is domain-specific to the adapter layer. A stronger argument can be made for placing it in src/business/adapters/ alongside the files that use it, following the same rationale as parseHl7DateToIso.ts which is colocated in src/business/adapters/ rather than extracted to a helpers layer.
If src/helpers/ is introduced, it must be a flat directory (no subdirectories) per UpFront rules, which the plan correctly states. But the decision to create a new top-level directory for a single file used exclusively by one adapter file deserves justification beyond what the plan provides. The plan should either justify src/helpers/ explicitly or move nhrMonitoring.ts to src/business/adapters/nhrMonitoring.ts.
2. sendMonitoringEvent is not in package.json dependencies — import will fail at runtime or type-check
monolith_DEPRECATED/services/monitoring is imported in nhrMonitoring.ts but is not declared as a dependency in /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/package.json. The plan references that "other monolith_DEPRECATED imports present in this package" justify availability, but looking at the actual source files, monolith_DEPRECATED imports appear in DmpNhrAdapter.ts (line 11: monolith_DEPRECATED/types/doctor/api/Patient) and useDmpNhr.ts. Whether those resolve as module-level dependencies or via monorepo path aliases needs confirming before stating it works. The plan should include an explicit step to verify the alias is available to this package for a runtime import (not just a type import), since monolith_DEPRECATED/types/... is type-only but monolith_DEPRECATED/services/monitoring is a value import.
3. NhrResponseActionFields for get_documents_content is missing error_code and error_message
The NhrResponseBase type defines error_code?: string and error_message?: string as fields on all response events. In withObservability, the catch block sends:
sendNhrResponse({
...
status: 'error',
error_message: error instanceof Error ? error.message : 'unknown_error',
} as NhrResponseDetails)
But for get_documents_content, the catch branch sends an error_message that is not present in NhrResponseActionFields['get_documents_content']. The error_message in the catch branch is injected as a bare property on the spread object before the cast. This is fine because it is part of NhrResponseBase, but buildSuccessFields for get_documents_content does not include error_code or error_message. That is intentional and correct — those are only needed on error paths, which are covered by the catch block in withObservability.
However, the plan states for the event table that error_code and error_message are "Response-Only Fields — All Response Events", but then notes "Not emitted for update_document_metadata". The types are inconsistent with this claim: NhrResponseActionFields['update_document_metadata'] includes status but neither error_code nor error_message. The get_documents_content action's buildSuccessFields also does not include error_code. Since both are in NhrResponseBase (not in the action-specific fields), they are always structurally available via the catch path. This is actually sound, but the plan's event table documentation does not make this clear and may confuse implementers.
Warnings (should fix)
4. Discriminated union members for NhrRequestDetails use optional fields where the event design table specifies required fields
The event design section states:
get_documents_contentrequest:requested_count— "Number of unique IDs requested — available at request time"update_document_metadatarequest:payloads_count,property_names— always available fromprops.params
Yet in the NhrRequestDetails union:
| (NhrRequestBase & { action: 'get_documents_content'; requested_count?: number })
| (NhrRequestBase & { action: 'update_document_metadata'; payloads_count?: number; property_names?: string[] })
All action-specific request fields are optional (?). The same pattern carries into NhrRequestActionFields. Since the call sites in NhrAdapterWithObservability always provide these values, making them required at the type level would enforce correctness at every call site (including any future adapter). The optionality only exists because withObservability accepts NhrRequestActionFields[TAction] generically and search_documents has no action-specific fields (Record<string, never>). The simplest fix is to keep optionality in the mapped types but make the union members required for fields that are always available. Alternatively, accept the optionality as a pragmatic tradeoff — but the plan should acknowledge this explicitly rather than leaving a mismatch with the event design table.
5. NhrResponseActionFields['get_documents_content'] is missing error_code
The event design table says error_code applies to "all response events" but is undefined on success and only set on error. For search_documents, buildSuccessFields explicitly maps error_code: result.status === 'error' ? result.code : undefined. For get_documents_content, DmpNhrAdapter.getDocumentsContent throws on error (it does not return an error-status result the way searchDocuments does), so the catch block handles the error path. This is structurally sound but is an asymmetry that should be called out in the plan, especially given searchDocuments has a dual return/throw error path. The plan does not explain why searchDocuments maps error_code inside buildSuccessFields while get_documents_content does not — this will confuse the implementer and anyone reviewing the monitoring events in Datadog.
6. useDmpNhr.ts import path for NhrAdapterWithObservability should use an alias
The plan specifies:
import { NhrAdapterWithObservability } from '../business/adapters/NhrAdapterWithObservability'
useDmpNhr.ts is at src/hooks/useDmpNhr.ts. The relative path ../business/adapters/... is a 2-level traversal, which falls below the 3+ ../ threshold for requiring an alias. This is technically compliant with the UpFront alias rule. No change required, but the plan should not describe this as a "one-line addition" — there is also the import to add, which is two lines (import + useMemo change).
7. NhrAdapterWithObservability is not exported from src/index.ts — plan correctly documents the rationale, but the implementation step does not include an explicit no-export note
The plan correctly states the class should not be exported. Implementation step 2 says "Create NhrAdapterWithObservability.ts" but does not include a reminder to leave src/index.ts untouched. Given that DmpNhrAdapter is exported from src/index.ts and the decorator wraps it, there is a risk a future developer adds the decorator to the public API. The plan should include an explicit step: "Do not add NhrAdapterWithObservability to src/index.ts."
**8. propertyName filter in `updateDocumentMetadata
…(truncated, 5265 more chars)