Project: doctolib-doctolib · Branch: .invalid · Mode: · Model: claude-sonnet-4-6 Stats: 1 user messages, 14 tool calls, tools used: Bash, Read. Conversation Turn 1 — User Review the TypeScript/architecture patterns in PR branch fhir-2213_dmp-api-typed-error-model in /Users/USER/workspace/doctolib/doctolib. Context: This PR adds typed error handling to @doctolib/dmp. Run git diff main...fhir-2213_dmp-api-typed-error-model to see all changes. Key changed files: - packages/@doctolib/dmp/src/business/DmpApiError.ts (new) — class extending Error, follows DmpAuthError pattern in src/api/auth-types.ts - packages/@doctolib/dmp/src/types/DmpApi.ts — new DmpApiErrorInfo interface; extended SearchResult - packages/@doctolib/dmp/src/types/FrDmpTypes.ts — extended DmpResponse with 4 new optional string fields - packages/@doctolib/dmp/src/business/DmpApi.ts — updated toSearchResult() private method - packages/@doctolib/dmp/src/index.ts — new exports Please review for: TypeScript typing quality, naming conventions, separation of concerns, interface design, and any UpFront/Doctolib architecture pattern violations. Turn 1 — Assistant I'll start by examining the git diff and the relevant files to conduct a thorough review. Tools used: - Bash: cd /Users/USER/workspace/doctolib/doctolib && git diff main...fhir-2213_dmp-api-typed-error-model -- 'packages/@doctolib/dmp//.ts' 'packages/@doctolib/dmp//.tsx' Tool results: → result (ERROR): Exit code 128 fatal: bad revision 'main...fhir-2213_dmp-api-typed-error-model' Tools used: - Bash: cd /Users/USER/workspace/doctolib/doctolib && git branch -a | grep fhir-2213 Tool results: → result (ok): fhir-2213_dmp-api-typed-error-model remotes/origin/fhir-2213_dmp-api-typed-error-model Tools used: - Bash: cd /Users/USER/workspace/doctolib/doctolib && git log --oneline main..fhir-2213_dmp-api-typed-error-model 2>/dev/null || git log --oneline master..fhir-2213_dmp-api-typed-error-model 2>/dev/null …(truncated, 10 more chars) Tool results: → result (ok): 5a3978c5b671 feat(FHIR-2213): add DmpApiError class and propagate DMP error fields through SearchResult Tools used: - Bash: cd /Users/USER/workspace/doctolib/doctolib && git diff master...fhir-2213_dmp-api-typed-error-model -- 'packages/@doctolib/dmp//.ts' 'packages/@doctolib/dmp//.tsx' Tool results: → result (ok): diff --git a/packages/@doctolib/dmp/src/business/DmpApi.test.ts b/packages/@doctolib/dmp/src/business/DmpApi.test.ts index 4ebfc45db572..777c0c9ef940 100644 --- a/packages/@doctolib/dmp/src/business/DmpApi.test.ts +++ b/packages/@doctolib/dmp/src/business/DmpApi.test.ts @@ -990,6 +990,77 @@ describe('DmpApi', () => { expect(result).toEqual({ documents: [] }) }) + it('should return SearchResult with error when findDocuments returns s_status ERROR', async () => { + const mock …(truncated, 10127 more chars) Now let me examine the existing files for context — particularly the existing patterns in DmpApi.ts, the auth-types.ts mentioned as a reference, and the full DmpApi.ts business file. Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/business/DmpApi.ts (1–60) 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 { blobToBase64 } from 'monolith_DEPRECATED/shared/BlobHelper' 5 import { getLoincCode, titlesByTypeAndKind } from 'monolith_DEPRECATED/models/document/document' 6 import { isMinor } from 'monolith_DEPRECATED/models/patient/patient' 7 import { isMedicalSynthesis } from 'monolith_DEPRECATED/c …(truncated, 1887 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/types/DmpApi.ts Tool results: → result (ok): 1 import type { Patient } from 'monolith_DEPRECATED/types/doctor/api/Patient' 2 import type { ClinicalDocument } from 'engines/cdar2/types/ClinicalDocument' 3 import type { DmpDocumentToAdd, DmpDocumentToDelete } from '../business/FrDmpConfig' 4 import type { DmpWorkflow, BillingSituation, GetDocumentsContentParams } from './FrDmpTypes' 5 6 export type PatientConsent = 'error' | 'authorized' | 'expired' | 'forbidden' | 'noAuthorization' 7 8 export type PatientConsentUpdate = 'add' | 'remove' 9 …(truncated, 5142 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/business/DmpApiError.ts Tool results: → result (ok): 1 import type { DmpApiErrorInfo } from '../types/DmpApi' 2 3 export class DmpApiError extends Error { 4 readonly apiErrorCode: string | undefined 5 6 readonly apiErrorType: string | undefined 7 8 readonly apiErrorContext: string | undefined 9 10 readonly apiErrorDescription: string | undefined 11 12 readonly apiErrorExtendedInformations: string | undefined 13 14 readonly apiErrorTypeDescription: string | undefined 15 16 readonly status: string 17 18 constructor(info: DmpA …(truncated, 652 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/types/FrDmpTypes.ts (1–220) Tool results: → result (ok): 1 import type { MedicalFolder } from '@doctolib/ehr-core' 2 import type { DmpResult } from '@doctolib/tpf-french-teleservices' 3 import type { Patient } from 'monolith_DEPRECATED/types/doctor/api/Patient' 4 import type { DmpDocumentToAdd, FrDmpDocument } from '../business/FrDmpConfig' 5 6 export interface FrDmpSessionStorage { 7 sessionId: string 8 practiceIndex: number 9 healthcareSettings: string 10 createdAt: Date 11 } 12 13 // DMP Requests 14 export interface DmpRequest { 15 s_co …(truncated, 7005 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/business/DmpApi.ts (490–590) Tool results: → result (ok): 490 } 491 492 private toSearchResult = (response: FindDocumentsResponse): SearchResult => ({ 493 documents: (response.Documents ?? []).map((doc) => ({ 494 uniqueId: doc.s_uniqueId, 495 title: doc.s_title, 496 description: doc.s_description, 497 classCode: doc.s_classCode, 498 typeCode: doc.s_typeCode, 499 creationDate: doc.s_creationDate, 500 serviceStartDate: doc.s_serviceStartDate, 501 serviceStopDate: doc.s_serviceStopDate, 502 he …(truncated, 3809 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/index.ts Tool results: → result (ok): 1 export { DmpApiError } from './business/DmpApiError' 2 export type { DmpApiErrorInfo } from './types/DmpApi' 3 4 export { 5 type DmpDocumentItem, 6 type DmpDocumentToAdd, 7 type DmpDocumentToDelete, 8 type FrDmpDocument, 9 ItemType, 10 type UploadedDmpDocumentItem, 11 } from './business/FrDmpConfig' 12 export { getErrorMessage, getWarningMessage } from './business/DmpI18nMessage' 13 export type { 14 DmpApi, 15 DmpAuthor, 16 DmpDocument, 17 DmpDocumentContent, 18 DmpSetUp …(truncated, 5545 more chars) Now let me examine the DmpAuthError pattern in auth-types.ts since it's cited as the reference model: Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/api/auth-types.ts Tool results: → result (ok): 1 export type DmpConnection = { 2 sessionId: string 3 healthcareSettings: string 4 } 5 6 export type PinValidationResult = { success: true } | { success: false; blocked: boolean; remainingAttempts?: number } 7 8 export type DmpAuthErrorCode = 9 | 'PIN_CANCELLED' 10 | 'PIN_BLOCKED' 11 | 'NO_CPX_READER' 12 | 'NO_LOCATION' 13 | 'NO_SITUATION' 14 | 'NO_LOCATION_MATCH' 15 | 'INCOMPLETE_CPX' 16 | 'SESSION_ERROR' 17 | 'CPX_CARD_ERROR' 18 | 'READER_ERROR' 19 | 'CPX_READ_ERROR …(truncated, 246 more chars) Tools used: - Bash: ls /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/business/ Tool results: → result (ok): CallDmpDelegate.test.ts CallDmpDelegate.ts DmpApi.test.ts DmpApi.ts DmpApiError.test.ts DmpApiError.ts DmpI18nMessage.ts DmpSharingSettings.test.ts DmpSharingSettings.ts FrDmpCommunication.test.ts FrDmpCommunication.ts FrDmpConfig.ts FrDmpConstants.ts frDmpPayloadBuilders.ts icanopee mapCpsToPscPayload.test.ts mapCpsToPscPayload.ts Tools used: - Bash: ls /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/ Tool results: → result (ok): api business components data helpers hooks index.ts pubsub test-helpers test-index.ts types Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/business/DmpApiError.test.ts Tool results: → result (ok): 1 import { DmpApiError } from './DmpApiError' 2 import type { DmpApiErrorInfo } from '../types/DmpApi' 3 4 describe('DmpApiError', () => { 5 describe('message assembly', () => { 6 it('combines description and extended info when both are present', () => { 7 const info: DmpApiErrorInfo = { 8 apiErrorDescription: 'Technical software error', 9 apiErrorExtendedInformations: 'Operation timed out.', 10 status: 'ERROR', 11 } 12 const error = new DmpApiErr …(truncated, 2610 more chars) Now I have a thorough picture of the changes. Here is the complete review: --- Architecture Review: fhir-2213_dmp-api-typed-error-model Summary of Changes The PR adds typed DMP API error handling by introducing DmpApiError (an Error subclass), DmpApiErrorInfo (an interface), extends SearchResult with an optional error field, adds four missing fields to DmpResponse, and updates toSearchResult() to populate the new field. --- Critical Issues (must fix) 1. DmpApiError class placed in the wrong directory /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/business/DmpApiError.ts is placed in src/business/. However, src/business/ in the UpFront convention is reserved for pure business logic and domain types — not for Error subclasses whose primary role is to serve as a transportable value across layer boundaries. More concretely, DmpApiError is not consumed anywhere in src/business/ in this PR; it is only exported from src/index.ts as a public API type for callers outside the package. Its natural home is src/api/, alongside DmpAuthError in auth-types.ts, which is the file explicitly cited by the PR as the pattern being followed. DmpAuthError lives in /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/api/auth-types.ts — an api/ layer file, not business/. Placing DmpApiError in business/ while its counterpart DmpAuthError lives in api/ creates an inconsistency in the very pattern this PR claims to follow. Recommended fix: Move DmpApiError.ts and DmpApiError.test.ts to src/api/, and update the import in DmpApiError.ts accordingly (from '../types/DmpApi' to '../types/DmpApi' which doesn't change, but the file itself moves). The src/index.ts export path would change from './business/DmpApiError' to './api/DmpApiError'. 2. DmpApiErrorInfo interface placed in src/types/DmpApi.ts instead of alongside the class it describes DmpApiErrorInfo is a data-shape interface that exclusively describes the constructor input of DmpApiError. When a type exists solely to type a class that lives in src/api/, it should either live in the same file as that class or in a src/api/ types file — not in src/types/DmpApi.ts, which is the public API contract interface file for DmpApi (the DmpApiImpl). Putting error metadata there conflates two different concerns: the API contract for the search() method and the constructor signature of an error class. Additionally, src/types/DmpApi.ts already exports DmpApiErrorInfo transitively via SearchResult.error?: DmpApiErrorInfo. Having DmpApiErrorInfo in DmpApi.ts with a direct export type { DmpApiErrorInfo } in index.ts is fine from a surfacing standpoint, but the location signals that this type belongs to the API method contract, when it actually belongs to the error class. Recommended fix: Either co-locate DmpApiErrorInfo in the same file as DmpApiError (in src/api/), or move it to src/api/auth-types.ts alongside DmpAuthError and DmpAuthErrorCode. The SearchResult.error field can then import it from '../api/...'. --- Warnings (should fix) 3. Message construction bug when apiErrorDescription is undefined but apiErrorExtendedInformations is present In /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/business/DmpApiError.ts at lines 19–21: const message = info.apiErrorExtendedInformations ? ${info.apiErrorDescription}: ${info.apiErrorExtendedInformations} : info.apiErrorDescription ?? 'DMP API error' If apiErrorExtendedInformations is present but apiErrorDescription is undefined, the message becomes "undefined: Operation timed out.". The extended informations check should be guarded on both fields being present: const message = info.apiErrorDescription && info.apiErrorExtendedInformations ? ${info.apiErrorDescription}: ${info.apiErrorExtendedInformations} : info.apiErrorDescription ?? info.apiErrorExtendedInformations ?? 'DMP API error' There is no test case covering this scenario (apiErrorExtendedInformations set, apiErrorDescription absent), which allowed the bug to go undetected. 4. status field typed as string instead of a union or literal type In DmpApiErrorInfo and correspondingly on DmpApiError, status is typed as string. The existing codebase uses STATUS_OK ('OK') as the only documented success value; the error path currently hard-codes 'ERROR' as fallback. Using bare string loses all static exhaustiveness checking. The downstream FrDmpTypes.ts already models s_status?: string (inherently loose from the transport layer), but the mapped domain type DmpApiErrorInfo is an ideal place to tighten this: export type DmpApiStatus = 'OK' | 'ERROR' | string // or a narrower union if all values are known At minimum, consider 'ERROR' | (string & {}) to keep open extensibility while signaling intent. If the full set of non-OK statuses from the DmpConnect-JS2 API is known, an exhaustive union should be used. 5. FindDocumentsResponse not extended with the four new fields — DmpResponse was extended instead The four new fields (s_apiErrorContext, s_apiErrorDescription, s_apiErrorExtendedInformations, s_apiErrorTypeDescription) were added to DmpResponse in FrDmpTypes.ts. This is technically correct since FindDocumentsResponse extends DmpResponse, but it means every response type that extends DmpResponse now implicitly declares these fields — including CpxStatusResponse, SessionStateResponse, PcscReadersResponse, etc. — even though error context fields are only consumed for findDocuments responses. If these fields are genuinely part of the DmpConnect-JS2 base response protocol for all commands, the placement in DmpResponse is correct and no change is needed. But the PR description says "add DmpApiError class and propagate DMP error fields through SearchResult" — if these fields are specific to findDocuments, they belong on FindDocumentsResponse directly. This warrants a clarifying comment on DmpResponse if the intent is that all commands can return these error fields. 6. Export ordering inconsistency in src/index.ts In /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/index.ts, DmpApiError and DmpApiErrorInfo are placed at the very top of the file (lines 1–2), before all other exports. The existing export structure groups by concern: business types, events, hooks, components, API auth. The new exports would naturally belong in the // DMP Authentication API section (lines 78–80) alongside DmpAuthError, or in a new dedicated // DMP API Errors section adjacent to it. Placing them at the top of the file without a comment suggests they are the primary exports of the package, which they are not. --- Suggestions (consider improving) 7. DmpApiError properties duplicate DmpApiErrorInfo fields verbatim DmpApiError re-declares all seven fields from DmpApiErrorInfo as class properties. Consider storing the original info object directly and delegating property access, or using Object.assign — this reduces duplication when the interface evolves. That said, the explicit readonly declarations do make the public surface of the class self-documenting, so this is a style tradeoff, not a correctness issue. 8. The toSearchResult() error block does not use DmpApiError — consider whether this is intentional toSearchResult() constructs a plain DmpApiErrorInfo object literal and attaches it to SearchResult.error. DmpApiError itself is never constructed inside the package. This means the class is only useful to callers who want to re-throw — it's a utility class, not an internal mechanism. That is a valid design, but it should be documented: a JSDoc comment on DmpApiError explaining that callers should construct it from SearchResult.error …(truncated, 606 more chars)