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

Stats: 1 user messages, 14 tool calls, tools used: Read, Glob, Grep.

Conversation

Turn 1 — User

Please review the technical documentation quality of this implementation plan:
/Users/USER/Library/CloudStorage/<REDACTED>/My Drive/ai-artifacts/nhr-document-list/observability/implementation-plan.md

This document is an implementation plan for adding Datadog observability to the NHR (National Health Records) adapter layer in Doctolib's monorepo. It is intended to be read by senior software engineers on the @doctolib/fire team who will implement the changes.

What I need from you:
1. Is the Context section clear enough for someone new to this area? Does it establish WHY this is needed?
2. Are the decisions (subfolder vs package, decorator pattern) well-justified? Are trade-offs clear?
3. Is the event schema table (fields, types, descriptions) complete and precise?
4. Are the PII exclusion rules clear and complete?
5. Is the code in the plan readable and well-structured?
6. Is the testing strategy actionable and specific enough?
7. Are there any gaps: missing sections, ambiguous steps, or unclear terminology?
8. Is the document well-organized for its audience (senior engineers implementing it)?

Read the file and provide specific, actionable feedback with suggested improvements for each issue you find. Focus on completeness, clarity, and correctness of the technical documentation — not on style preferences.

Turn 1 — Assistant

Let me read the implementation plan file.

Tools used:

- Read: /Users/USER/Library/CloudStorage/<REDACTED>/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, 19463 more chars)

Now let me look at the actual source files referenced in the plan to verify technical accuracy.

Tools used:

- Glob: packages/@doctolib/national-health-records/src//.ts in /Users/USER/workspace/doctolib/doctolib

Tools used:

- Glob: packages/@doctolib/tpf-national-health-records/src//.ts in /Users/USER/workspace/doctolib/doctolib

Tool results:

  → result (ok): No files found

Tool results:

  → result (ok): packages/@doctolib/national-health-records/src/business/mappers/toNhrPatientInformation.ts
packages/@doctolib/national-health-records/src/mocks/nhrMockedColumns.ts
packages/@doctolib/national-health-records/src/variants/dmp/index.ts
packages/@doctolib/national-health-records/src/variants/useNhrDocumentListVariant.ts
packages/@doctolib/national-health-records/src/mocks/filterMockedDocuments.test.ts
packages/@doctolib/national-health-records/src/mocks/filterMockedDocuments.ts
packages/@doctolib/na
…(truncated, 3171 more chars)

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)

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:

- Glob: packages/@doctolib/tpf-national-health-records//.ts in /Users/USER/workspace/doctolib/doctolib

Tools used:

- Glob: monolith_DEPRECATED/services/monitoring in /Users/USER/workspace/doctolib/doctolib

Tool results:

  → result (ok): No files found

Tool results:

  → result (ok): No files found

Tools used:

- Glob: /tpf-national-health-records/ in /Users/USER/workspace/doctolib/doctolib

Tools used:

- Grep: sendMonitoringEvent in /Users/USER/workspace/doctolib/doctolib

Tool results:

  → result (ok): Found 10 files limit: 10
packages/@doctolib/dmp/src/business/DmpApi.ts
app/assets/javascripts/services/doctor/SmartFoldersService.ts
app/assets/javascripts/services/doctor/SmartFoldersService.test.ts
app/assets/javascripts/modules/doctor/mobile/document-upload/NewDocumentModule/variations/useTrackDocumentUploadVariation.ts
app/assets/javascripts/modules/doctor/mobile/DocumentEditModule/variations/useTrackDocumentUpdatedVariation.ts
app/assets/javascripts/entrypoints-content/doctor-desktop.tsx
ap
…(truncated, 312 more chars)

Tool results:

  → result (ok): .yalc/@doctolib/tpf-national-health-records/CHANGELOG.md
.yalc/@doctolib/tpf-national-health-records/dist/index.d.ts.map
.yalc/@doctolib/tpf-national-health-records/dist/index.d.ts
.yalc/@doctolib/tpf-national-health-records/src/index.ts
.yalc/@doctolib/tpf-national-health-records/package.json
.yalc/@doctolib/tpf-national-health-records/README.md
.yalc/@doctolib/tpf-national-health-records/dist/index.js
.yalc/@doctolib/tpf-national-health-records/dist/index.js.map
.yalc/@doctolib/tpf-national-he
…(truncated, 7811 more chars)

Tools used:

- Read: /Users/USER/workspace/doctolib/doctolib/.yalc/@doctolib/tpf-national-health-records/src/business/types/service.ts

Tool results:

  → result (ok): 1	import type { NhrDocumentSearchProps, NhrDocumentSearchResult } from '#business/types/document'
2	
3	export type CountryCode = 'fr' | 'mock'
4	
5	export type NhrService = {
6	  searchDocuments(props: NhrDocumentSearchProps): Promise<NhrDocumentSearchResult>
7	}
8	
9	export type NhrAdapter = {
10	  searchDocuments(props: NhrDocumentSearchProps): Promise<NhrDocumentSearchResult>
11	}
12

Tools used:

- Grep: sendMonitoringEvent in /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/business/DmpApi.ts

Tool results:

  → result (ok): 1-/ eslint-disable @typescript-eslint/no-non-null-assertion /
2-import type { Patient } from 'monolith_DEPRECATED/types/doctor/api/Patient'
3-import type { MedicalFolder } from '@doctolib/ehr-core'
4-import { getLoincCode } from 'monolith_DEPRECATED/models/document/document'
5:import { sendMonitoringEvent } from 'monolith_DEPRECATED/services/monitoring'
6-import { isMinor } from 'monolith_DEPRECATED/models/patient/patient'
7-import { isMedicalSynthesis } from 'monolith_DEPRECATED/components/do
…(truncated, 763 more chars)

Tools used:

- Read: /Users/USER/workspace/doctolib/doctolib/node_modules/@doctolib/tpf-national-health-records/src/business/types/service.ts

Tool results:

  → result (ok): 1	import type {
2	  NhrDocumentContentResult,
3	  NhrDocumentMetadataUpdateProps,
4	  NhrDocumentMetadataUpdateResult,
5	  NhrDocumentSearchProps,
6	  NhrDocumentSearchResult,
7	  NhrGetDocumentsContentProps,
8	} from '#business/types/api'
9	
10	export type CountryCode = 'fr' | 'mock'
11	
12	export type NhrService = {
13	  searchDocuments(props: NhrDocumentSearchProps): Promise<NhrDocumentSearchResult>
14	  getDocumentsContent(props: NhrGetDocumentsContentProps): Promise<NhrDocumentContentResult
…(truncated, 452 more chars)

Tools used:

- Read: /Users/USER/workspace/doctolib/doctolib/node_modules/@doctolib/tpf-national-health-records/src/index.ts

Tool results:

  → result (ok): 1	export { createNhrService } from '#business/nhr-service-factory'
2	
3	export type {
4	  // api
5	  NhrResult,
6	  NhrResultStatus,
7	  NhrDocumentSearchParams,
8	  NhrDocumentSearchProps,
9	  NhrDocumentSearchResult,
10	  NhrGetDocumentsContentParams,
11	  NhrGetDocumentsContentProps,
12	  NhrDocumentContentResult,
13	  NhrDocumentMetadataUpdatePayload,
14	  NhrDocumentMetadataUpdateParams,
15	  NhrDocumentMetadataUpdateProps,
16	  NhrDocumentMetadataUpdatePayloadResult,
17	  NhrDocumentMetada
…(truncated, 597 more chars)

Tools used:

- Read: /Users/USER/workspace/doctolib/doctolib/node_modules/@doctolib/tpf-national-health-records/src/business/types/api.ts

Tool results:

  → result (ok): 1	import type { DateRange, NhrDocument, NhrDocumentFile } from '#business/types/document'
2	import type { NhrPatientInformation } from '#business/types/participant'
3	
4	export type NhrResultStatus = 'success' | 'skipped' | 'error' | 'partial success'
5	
6	export type NhrResult = {
7	  status: NhrResultStatus
8	  message?: string
9	  code?: string
10	}
11	
12	// Document Search
13	export type NhrDocumentSearchParams = {
14	  formats?: string[]
15	  categories?: string[]
16	  status?: string
17	 
…(truncated, 1663 more chars)

Now I have everything I need to provide a thorough and technically grounded review. Let me compile my findings.

---

Here is the full review. Issues are organized by the eight questions you asked, ordered from most critical to least.

---

Overall Assessment

The document is well-structured, honest about its constraints, and clearly targeted at engineers who will implement it. The decorator pattern choice is well-argued, and the data flow diagram is genuinely useful. However, there are several correctness issues in the code, a critical mismatch between the schema table and the actual type definitions, and gaps in the testing strategy that would leave the implementation team guessing.

---

1. Is the Context section clear enough?

Verdict: Mostly yes, but two things are missing.

The "zero observability" framing is clear and the constraint list is helpful. Two gaps:

Gap 1 — No stated trigger or urgency. A senior engineer joining the team reads this and has no idea whether this work is driven by an incident, a new product feature launching, a compliance requirement, or a quarterly OKR. Adding one sentence — even "This work is triggered by the upcoming FR/DE adapter rollout and the inability to diagnose DMP API degradation incidents" — anchors the effort in a real need.

Gap 2 — @doctolib/ti_connect reference is unverified. The plan says to follow patterns from @doctolib/ti_connect, but never links to it or shows what those patterns look like. If the implementing engineer has not worked on ti_connect, they have no way to validate the pattern without searching the codebase. Either inline the ti_connect event structure as a reference, or link to the file path.

---

2. Are the decisions well-justified?

Verdict: Yes — the subfolder vs package decision is the strongest section in the document.

The trade-off table is complete, the YAGNI argument is correctly applied, and the "when to extract" trigger (second adapter) is explicit. No changes needed here.

The decorator pattern rationale is also solid. One small addition would help: state explicitly that createNhrService in @doctolib/tpf-national-health-records accepts an NhrAdapter and does not depend on the concrete class, so the decorator slot is already available by design. This removes any doubt about whether adding the wrapper breaks the wiring.

---

3. Is the event schema table complete and precise?

Verdict: No — two correctness issues.

Issue 3a — status values do not match the actual type.

The schema table lists:
'success' | 'error' | 'partial_success' | 'skipped'

The actual NhrResultStatus type in @doctolib/tpf-national-health-records/src/business/types/api.ts is:
export type NhrResultStatus = 'success' | 'skipped' | 'error' | 'partial success'

Note 'partial success' (space, not underscore). The code in NhrAdapterWithObservability.ts uses result.status directly, so it will emit 'partial success' at runtime. The schema table and the type definition will be inconsistent from day one, which will confuse anyone writing Datadog monitors or dashboards. Pick one canonical value and make both the schema and the code agree.

Issue 3b — error_code is typed string | number | undefined but NhrResult.code is typed string | undefined.

From api.ts:
export type NhrResult = {
  status: NhrResultStatus
  message?: string
  code?: string
}

code is string, not string | number. The NhrResponseDetails type in the plan declares error_code?: string | number, which is wider than what result.code can actually hold. The description in the table says "e.g., DMP API error code" and implies it could be numeric, but that is not what the interface delivers. Either narrow the type to string | undefined to match the interface, or explain why the number branch exists (perhaps it comes from somewhere else).

Issue 3c — Missing field: documents_count on the success path for searchDocuments references result.documents.length, but when status is 'error', result.documents is []. This is fine, but the schema table does not note that documents_count will always be 0 on the error path. A dashboard reader will see documents_count: 0 on error events and may not realize this is expected. Add a note in the description column: "0 on error responses."

---

4. Are the PII exclusion rules clear and complete?

Verdict: Adequate, but one gap.

The exclusion table covers the obvious cases. The one gap: property_names in update_document_metadata contains values like 'status' or 'visibility', which are property names on the document schema. The plan notes these are "safe metadata" but does not explain why. A reader unfamiliar with the domain may wonder whether propertyName in NhrDocumentMetadataUpdatePayload could ever contain a patient-derived string. The code shows it's a schema field name, not patient data, but the plan should say that explicitly: "propertyName is always a fixed document schema field name, never a patient-supplied or patient-derived value."

Also note: the error_message field in the catch block uses error instanceof Error ? error.message : 'unknown_error'. The plan should acknowledge that this path is a potential PII leak vector if the underlying adapter ever throws an error whose message contains patient data (e.g., a poorly formatted DMP error that echoes back the INS). The PII section should either call this out as a known risk, or state that all DmpNhrAdapter errors are sanitized before throwing.

---

5. Is the code readable and well-structured?

Verdict: Mostly yes, with two correctness issues.

Issue 5a — getDocumentsContent does not handle partial results.

Looking at DmpNhrAdapter.getDocumentsContent: if the adapter succeeds, the decorator emits status: 'success' unconditionally. But requested_count vs returned_count can differ silently (the underlying DMP API may return fewer results than requested). The plan does not decide whether requested_count !== returned_count should produce status: 'partial_success' or just be observable via the count fields. Either decision is fine — but not documenting it means the implementing engineer will guess.

Issue 5b — The useMemo dependency array in the wiring section does not include observableAdapter.

The plan's "After" snippet creates observableAdapter inside useMemo but it remains correct because NhrAdapterWithObservability captures adapter and 'dmp' as constructor arguments, and adapter is already inside the memo. This is actually fine, but the code structure is slightly awkward — observableAdapter is a local variable whose construction is not shown as expensive. The plan could note why NhrAdapterWithObservability does not need to be memoized separately (it's a pure wrapper with no state of its own), or simplify to a single chain:

return createNhrService('fr', new NhrAdapterWithObservability(new DmpNhrAdapter(dmpApi, facets), 'dmp'))

This matches the existing code style better.

Issue 5c — NhrResponseDetails allows any combination of operation-specific fields. The flat type means TypeScript will not catch passing documents_count to an update_document_metadata response event. This is an underspecification issue, not a bug, but the plan describes the intent as "prevents accidental PII inclusion." Consider using discriminated unions keyed on action for stronger guarantees, or at minimum note in the plan that the flat type is a deliberate simplicity trade-off.

---

6. Is the testing strategy actionable and specific enough?

Verdict: Partially — the coverage list is good but lacks concrete test structure.

The coverage bullet list is specific and correct. But a senior engineer writing the tests for the first time needs more:

Gap 6a — No test scaffolding described. How is sendMonitoringEvent` mocke
…(truncated, 4938 more chars)