Project: doctolib-doctolib · Branch: .invalid · Mode:  · Model: claude-opus-4-7

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

Conversation

Turn 1 — User

Review recently modified code in the Doctolib monorepo at /Users/USER/workspace/doctolib/doctolib on branch fhir-2245_observability-search-documents for simplification opportunities. This is the third review pass.

Focus on these 5 files only:
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/business/adapters/NhrAdapterWithObservability.test.ts
4. packages/@doctolib/national-health-records/src/business/adapters/nhrMonitoring.test.ts
5. packages/@doctolib/national-health-records/src/hooks/useDmpNhr.ts

Context: The two-phase observe<TAction>(action).execute<TResult>(...) pattern is intentional — it constrains TypeScript inference at each call site via mapped types. The as NhrRequestDetails / as NhrResponseDetails casts inside execute() are necessary because TypeScript cannot narrow through spread on mapped index types. The .catch(() => {}) fire-and-forget pattern is intentional (matches ti_connect convention). Do NOT suggest removing these.

Identify only simplifications that reduce complexity without changing behaviour. Report with file:line or say "no simplifications needed".

Turn 1 — Assistant

I'll analyze the 5 files for simplification opportunities.

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	// ── Infrastructure base types ──
11	
12	type NhrRequestBase = {
13	  trace_id: str
…(truncated, 2533 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, 3641 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, 11267 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, 2497 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, 950 more chars)

I've reviewed all 5 files. Here are the simplification opportunities I found.

Findings

NhrAdapterWithObservability.ts

Lines 41–74: Duplicated infrastructure fields and duration_ms computation across three sendNhrResponse call paths.

The same { trace_id, adapter, action, phase: 'response', duration_ms: ... } is built three times — once in success, once in catch (and once for request). The performance.now() - start is duplicated in success and error paths. A single small helper inside execute would reduce repetition without changing behaviour or types.

Concretely, you could hoist the response-base into a closure:

const sendResponse = (fields: NhrResponseActionFields[TAction]): void => {
  sendNhrResponse({
    trace_id: traceId,
    adapter: this.adapterName,
    action,
    phase: 'response',
    duration_ms: performance.now() - start,
    ...fields,
  } as NhrResponseDetails)
}

Then the success branch becomes sendResponse(buildSuccessFields(result)) and the catch branch becomes sendResponse({ status: 'error', error_message: ... }). This collapses ~12 lines of duplication into a single helper while preserving the mapped-type constraint and the necessary as NhrResponseDetails cast (still confined to one place).

NhrAdapterWithObservability.test.ts

Lines 31–229: Per-test repetition of (mockAdapter.searchDocuments as jest.Mock).mockResolvedValue({ status: 'success', documents: [] }) and new NhrAdapterWithObservability(mockAdapter, 'dmp').

This identical pair appears in ~10 tests. Hoisting the wrapper into a let wrapper initialised in beforeEach, plus a tiny helper like:

function givenSuccessfulSearch(documents: object[] = []): void {
  ;(mockAdapter.searchDocuments as jest.Mock).mockResolvedValue({ status: 'success', documents })
}

would reduce a lot of boilerplate. The MOCK_INS-only call sites also repeat { patient: { nationalHealthIdentifier: MOCK_INS }, params: {} } — a defaultProps constant or searchWithDefaults(wrapper) helper would cut another ~7 repetitions. Behaviour is identical; readability improves.

nhrMonitoring.ts

Lines 72–78: sendNhrRequest and sendNhrResponse are near-duplicates differing only in the _request / _response suffix.

Two reasonable simplifications, both behaviour-preserving:

1. Inline the suffix using the literal phase field already in the payload:
      const send = (details: NhrRequestDetails | NhrResponseDetails): void => {
     sendMonitoringEvent({ name: nhr_adapter_${details.action}_${details.phase}, details }).catch(() => {})
   }
   export const sendNhrRequest = (d: NhrRequestDetails) => send(d)
   export const sendNhrResponse = (d: NhrResponseDetails) => send(d)

   This removes the duplicated sendMonitoringEvent(...).catch(() => {}) line. The two named exports stay as the public surface (preserving current call sites and tests).

2. Minor: lines 4–8 have two single-line type aliases each followed by a comment about future extensions. Those comments are fine; no change needed.

nhrMonitoring.test.ts

Lines 15–84: The two describe blocks are structurally identical — three tests each (event name, details passthrough, no-throw on rejection). A small describe.each over [sendNhrRequest, sendNhrResponse] with their expected suffix would halve the file. However, this trades local clarity (each test is currently self-contained and reads top-to-bottom) for less duplication. Given the project preference for "explicit code over overly compact code" stated in the system prompt, this is borderline — I'd leave it as-is. No change recommended.

useDmpNhr.ts

No simplifications needed. The hook is already minimal: two memos, a query, and a returned object. The eslint-disable-next-line react-hooks/exhaustive-deps on line 15 is intentional (DmpApi is created once on mount).

Summary

| File | Recommendation |
|---|---|
| NhrAdapterWithObservability.ts:41-74 | Extract sendResponse closure inside execute to remove duplicated infrastructure-fields/duration_ms block |
| NhrAdapterWithObservability.test.ts:31-229 | Hoist wrapper into beforeEach; add givenSuccessfulSearch and defaultProps helpers to cut repeated setup |
| nhrMonitoring.ts:72-78 | Collapse sendNhrRequest/sendNhrResponse bodies to a shared send helper using details.phase for the suffix |
| nhrMonitoring.test.ts | No change — current explicitness is preferred over describe.each consolidation |
| hooks/useDmpNhr.ts | No simplifications needed |

Files reviewed:
- /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/nhrMonitoring.ts
- /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.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/nhrMonitoring.test.ts
- /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/hooks/useDmpNhr.ts