Project: doctolib-doctolib · Branch: .invalid · Mode:  · Model: claude-sonnet-4-6

Stats: 1 user messages, 8 tool calls, tools used: Bash, Read.

Conversation

Turn 1 — User

Review the following files from PR #242170 for compliance with Doctolib's UpFront frontend architecture patterns. The repo is at /Users/USER/workspace/doctolib/doctolib.

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

Context:
- This is an increment that adds Datadog observability to the NHR adapter layer using the Decorator pattern
- File placement in src/business/adapters/ (collocated with DmpNhrAdapter.ts) was explicitly chosen over a new src/observability/ folder to follow UpFront layer taxonomy
- NhrAdapterWithObservability is NOT exported from src/index.ts intentionally — it's internal wiring only consumed by useDmpNhr.ts
- nhrMonitoring.ts is collocated as a single-consumer utility following existing patterns in the package

Check for: separation of concerns, naming conventions, component/class structure, anti-patterns, and UpFront layer taxonomy compliance. Report specific issues with file:line references.

Turn 1 — Assistant

I'll examine the diff and the relevant files to conduct a thorough architecture review.

Tools used:

- Bash: git diff HEAD~1 -- 'packages/@doctolib/national-health-records/src/business/adapters/nhrMonitoring.ts' 'packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.ts'
…(truncated, 362 more chars)

Tool results:

  → result (ok): diff --git a/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.ts b/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.ts
new file mode 100644
index 000000000000..0e9cca4fe59b
--- /dev/null
+++ b/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.ts
@@ -0,0 +1,114 @@
+import type {
+  NhrAdapter,
+  NhrDocumentSearchProps,
+  NhrDocumentSearchResult,
+  NhrGetDocumentsCo
…(truncated, 8771 more chars)

Now let me read the existing files for full context.

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 examine the surrounding package structure for context.

Tools used:

- Bash: find /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src -type f | sort

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)

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, 6018 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)

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, 9995 more chars)

I now have everything I need for a complete review. Here is the full findings report.

---

Architecture Review: PR #242170 — NHR Adapter Observability Layer

Critical Issues (must fix)

1. nhrMonitoring.ts is not a pure-function module — it crosses the src/business/ contract

File: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/nhrMonitoring.ts, line 1

import { sendMonitoringEvent } from 'monolith_DEPRECATED/services/monitoring'

src/business/ must contain pure functions and domain types only. sendNhrRequest and sendNhrResponse perform a side effect — they fire an async network call via sendMonitoringEvent. That is an I/O operation, not a pure transformation.

The correct placement depends on what sendMonitoringEvent is: if it is a platform-level API (which monolith_DEPRECATED/services/monitoring signals), then nhrMonitoring.ts belongs in src/helpers/ (stateless utility with a side effect acceptable there) or the send helpers should be extracted to their own file in src/helpers/. The type definitions (NhrRequestDetails, NhrResponseDetails, etc.) are pure and can stay in src/business/, but the send functions must not.

Recommended split:
- Keep types and mapped types in src/business/adapters/nhrMonitoring.ts (or move to a dedicated types file).
- Move sendNhrRequest / sendNhrResponse to src/helpers/nhrMonitoring.ts.

2. NhrAdapterWithObservability is exported from its file with a named export but imports an I/O helper that lives in src/business/

File: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.ts, line 10

import { sendNhrRequest, sendNhrResponse } from './nhrMonitoring'

This is a transitive consequence of issue #1. Once nhrMonitoring.ts is correctly split, this import must be updated to reference the helpers layer.

---

Warnings (should fix)

3. Naming inconsistency: nhrMonitoring.ts uses kebab-case naming for a multi-export file, but the exports it contains are a mix of types and functions with distinct concerns

File: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/nhrMonitoring.ts

nhrMonitoring.ts exports six types plus two functions. The file name itself is acceptable for a multi-export helper file, but the colocation inside src/business/adapters/ alongside DmpNhrAdapter.ts and NhrAdapterWithObservability.ts creates a flat directory that conflates three different roles: adapter implementations, infrastructure types, and monitoring side effects. When get_documents_content and update_document_metadata are instrumented in follow-up increments, this file will grow substantially and the mixed-concern problem will compound. Proactively separating the types file from the send-helpers file now avoids a larger refactor later.

4. NhrAdapterWithObservability is not exported from src/index.ts, but DmpNhrAdapter is — this asymmetry is unresolved

File: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/index.ts, line 14

export { DmpNhrAdapter } from './business/adapters/DmpNhrAdapter'

The PR description states that NhrAdapterWithObservability is intentionally internal. However, DmpNhrAdapter is exported from the public API of the package, which means external consumers can construct a DmpNhrAdapter directly — bypassing the observability wrapper entirely. If the wrapper is the correct production path (as enforced in useDmpNhr.ts), then re-exporting the raw adapter without the wrapper from the package surface is misleading. Either:
- Remove DmpNhrAdapter from src/index.ts if direct construction outside the package is not intended, or
- Export NhrAdapterWithObservability as well and document that consumers are expected to wrap their adapter.

The current state makes it trivially easy for a caller outside the package to instantiate DmpNhrAdapter alone and silently skip observability.

5. useDmpNhr.ts wires the decorator inline inside useMemo — object construction belongs outside the hook

File: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/hooks/useDmpNhr.ts, lines 21-24

const nhrService = useMemo(
  () => createNhrService('fr', new NhrAdapterWithObservability(new DmpNhrAdapter(dmpApi, facets), 'dmp')),
  [dmpApi, facets]
)

The useDmpNhr hook is now responsible for both adapter composition (choosing which concrete adapter to use, which observability decorator to apply, which adapter name string to pass) and service creation. That is three distinct decisions packed into one useMemo factory. When the third adapter method is instrumented, or when a feature flag is introduced to opt out of observability, this line will need modification inside the hook rather than at a factory/composition layer.

The preferred pattern for this package is to push adapter composition into a factory function — either a named function in useDmpNhr.ts itself or a separate src/business/adapters/createDmpNhrAdapter.ts. The hook should only call createNhrService('fr', createDmpNhrAdapter(dmpApi, facets)).

---

Suggestions (consider improving)

6. The two-phase observe().execute() API adds indirection that will be difficult to navigate when extending to additional actions

File: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.ts, lines 34-79

The curried builder pattern (observe(action, ins).execute(requestFields, operation, buildSuccessFields)) is clever, but it splits a single logical operation across two call sites in each adapter method. When getDocumentsContent and updateDocumentMetadata are instrumented, each will require understanding the two-phase API to read correctly. A flat observe(action, ins, requestFields, operation, buildSuccessFields) signature is less clever but immediately scannable. This is a readability trade-off, not an architecture violation — flag it for the team to decide.

7. as NhrRequestDetails and as NhrResponseDetails type assertions in execute() are a design smell

File: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.ts, lines 51 and 63

} as NhrRequestDetails)

} as NhrResponseDetails)

The comment acknowledges the casts are "confined to execute()," but casts on outgoing objects passed to typed functions indicate that the intermediate spread type does not satisfy the target. The comment says "every call site is fully type-checked via the mapped types" but the casts tell the opposite story — the compiler cannot verify the spread satisfies the discriminated union without the assertion. This is acceptable as a pragmatic workaround, but it means a future action that produces a mis-shaped payload will not be caught at compile time inside execute(). Document this risk explicitly or restructure the types so the spread is provably correct.

8. Test file for nhrMonitoring.ts is absent

The new nhrMonitoring.ts file exports two public functions — sendNhrRequest and sendNhrResponse — and two named constants that form the public API of the monitoring module. UpFront convention requires tests colocated with source files. The NhrAdapterWithObservability.test.ts tests sendMonitoringEvent indirectly, but sendNhrRequest and sendNhrResponse themselves (event name construction, error suppression) have no direct unit coverage.