Project: doctolib-doctolib · Branch: .invalid · Mode: · Model: claude-sonnet-4-6 Stats: 1 user messages, 27 tool calls, tools used: Bash, Read. Conversation Turn 1 — User Analyze test coverage for PR #240425 in the doctolib/doctolib repo (branch fhir-2213_dmp-api-typed-error-model). The git working directory is /Users/USER/workspace/doctolib/doctolib. Context: This PR implements FHIR-2213 — adding typed DMP API error handling to the @doctolib/dmp package. Changed files (run git diff main...fhir-2213_dmp-api-typed-error-model to see all): - packages/@doctolib/dmp/src/business/DmpApiError.ts (new) — typed error class - packages/@doctolib/dmp/src/business/DmpApiError.test.ts (new) — 7 unit tests - packages/@doctolib/dmp/src/business/DmpApi.ts — toSearchResult() updated to map error fields - packages/@doctolib/dmp/src/business/DmpApi.test.ts — 4 new tests added for error mapping - packages/@doctolib/dmp/src/types/DmpApi.ts — DmpApiErrorInfo interface + SearchResult.error? - packages/@doctolib/dmp/src/types/FrDmpTypes.ts — 4 new fields on DmpResponse - packages/@doctolib/dmp/src/index.ts — new exports Analyze test coverage quality and completeness. Are there missing edge cases or critical scenarios not covered? Focus only on the changed files. Turn 1 — Assistant I'll analyze the PR by examining the changed files systematically. Tools used: - Bash: cd /Users/USER/workspace/doctolib/doctolib && git diff main...fhir-2213_dmp-api-typed-error-model -- packages/@doctolib/dmp/src/business/DmpApiError.ts packages/@doctolib/dmp/src/business/DmpApiE …(truncated, 247 more chars) Tool results: → result (ERROR): Exit code 128 Tools used: - Bash: cd /Users/USER/workspace/doctolib/doctolib && git log --oneline main..fhir-2213_dmp-api-typed-error-model 2>/dev/null | head -20 Tool results: → result (ok): (Bash completed with no output) Tools used: - Bash: cd /Users/USER/workspace/doctolib/doctolib && git branch -a 2>/dev/null | 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 master..HEAD 2>/dev/null | head -20 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...HEAD -- packages/@doctolib/dmp/src/business/DmpApiError.ts packages/@doctolib/dmp/src/business/DmpApiError.test.ts packages/@doctol …(truncated, 218 more chars) 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 read the full source files for deeper analysis. 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/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) Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/business/DmpApi.ts (490–550) 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, 2159 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/types/FrDmpTypes.ts (185–215) Tool results: → result (ok): 185 / Event codes to filter by. Returns documents matching at least one event code. / 186 EventCodes?: EventCode[] 187 / Enable transaction frame debugging (0 or 1). Development only. / 188 i_getTransactionFrames?: number 189 } 190 191 // DMP Responses 192 export interface DmpResponse extends DmpResult { 193 // Value "OK" for success. 194 s_status?: string 195 i_apiErrorCode?: string 196 i_apiErrorType?: string 197 s_apiErrorContext?: string 198 s_apiErrorDescription?: …(truncated, 400 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 look at what consumers of SearchResult exist, to understand the integration surface, and look at the existing DmpApi.test.ts context around the added tests. Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/business/DmpApi.test.ts (960–1080) Tool results: → result (ok): 960 city: 'Paris', 961 postalCode: '75001', 962 state: 'IDF', 963 country: 'FR', 964 additionalLocator: '', 965 precinct: '', 966 postBox: '', 967 unitId: '', 968 }, 969 ], 970 telecoms: [{ type: 1, usage: 1, value: '+33600000000' }], 971 institution: { 972 id: 'inst-1', 973 name: 'Hôpital Central', 974 class …(truncated, 4311 more chars) Tools used: - Bash: cd /Users/USER/workspace/doctolib/doctolib && grep -r "SearchResult\|\.search(\|toSearchResult\|result\.error\|DmpApiError\|DmpApiErrorInfo" --include=".ts" --include=".tsx" -l packages/@doctol …(truncated, 69 more chars) Tool results: → result (ok): packages/@doctolib/dmp/src/index.ts packages/@doctolib/dmp/src/types/DmpApi.ts packages/@doctolib/dmp/src/business/DmpApi.ts packages/@doctolib/dmp/src/business/DmpApiError.ts Tools used: - Bash: cd /Users/USER/workspace/doctolib/doctolib && grep -r "\.search(\|SearchResult\|result\.error\|DmpApiError\|DmpApiErrorInfo" --include=".ts" --include=".tsx" -l packages/ engines/ app/ 2>/dev/n …(truncated, 74 more chars) Tool results: → result (ok): packages/@doctolib/secure_messaging_inbox/src/components/doctor/desktop/shared/MailModal/Recipients/RecipientAutoComplete/RecipientAutoComplete.tsx packages/@doctolib/secure_messaging_inbox/src/components/doctor/desktop/fr/ConfigurationScreen/ApicryptLogin/ApicryptLogin.tsx packages/@doctolib/patient_account_documents/src/queries/patient_account_documents/appointments/documents.ts packages/@doctolib/documents-ui/src/services/fetchDocument.test.ts packages/@doctolib/patient_messaging_pro_sdk/src/ …(truncated, 2028 more chars) Tools used: - Bash: cd /Users/USER/workspace/doctolib/doctolib && grep -r "DmpApi\|dmpApi\.search\|SearchResult" --include=".ts" --include=".tsx" -l packages/ engines/ 2>/dev/null | grep -v "packages/@doctolib/dmp …(truncated, 37 more chars) Tool results: → result (ok): packages/@doctolib/secure_messaging_inbox/src/components/doctor/desktop/shared/MailModal/Recipients/RecipientAutoComplete/RecipientAutoComplete.tsx packages/@doctolib/patient_messaging_pro_sdk/src/modules/doctor_app/MessageDocumentsModule/EhrDocumentAttachmentModalModule/TextExcerptHighlighterModule/useTextExcerptHighlighterModuleController.ts packages/@doctolib/patient_messaging_pro_sdk/src/modules/doctor_app/MessageDocumentsModule/EhrDocumentAttachmentModalModule/EhrDocumentAttachmentContentMo …(truncated, 2005 more chars) Tools used: - Bash: cd /Users/USER/workspace/doctolib/doctolib && grep -r "from '@doctolib/dmp'" --include=".ts" --include=".tsx" -l 2>/dev/null | grep -v "node_modules" | xargs grep -l "search\|SearchResult" 2>/d …(truncated, 18 more chars) Tool results: → result (ok): app/assets/javascripts/components/doctor/desktop/routes/BMSApp.tsx packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.test.ts packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts packages/@doctolib/national-health-records/src/hooks/useDmpNhr.test.ts packages/@doctolib/national-health-records/src/hooks/useDmpNhr.ts 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 { 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 type { 14 Nh …(truncated, 5904 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 { DmpNhrAdapter } from '../business/adapters/DmpNhrAdapter' 6 import type { UseNhrOperations } from './useNationalHealthRecords' 7 8 export const useDmpNhr = ({ patient }: { patient: Patient }): UseNhrOperations => { 9 const { dmpSet …(truncated, 534 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.test.ts Tool results: → result (ok): 1 import { getPatientInsForDMP } from '@doctolib/dmp' 2 import type { DmpApi, DmpDocumentContent, SearchResult as DmpSearchResult } from '@doctolib/dmp' 3 import type { NhrDocumentSearchProps } from '@doctolib/tpf-national-health-records' 4 import { DmpNhrAdapter } from './DmpNhrAdapter' 5 6 jest.mock('@doctolib/dmp', () => ({ 7 getPatientInsForDMP: jest.fn(), 8 DOCUMENT_STATUS: { validated: 1, obsolete: 2, archived: 4 }, 9 DOCUMENT_VISIBILITY: { 10 sharedWithAll: 1, 11 notSharedW …(truncated, 13663 more chars) Now let me look at the STATUS_OK constant and the findDocumentsResponseFixtureBuilder to understand edge cases around status handling. Tools used: - Bash: cd /Users/USER/workspace/doctolib/doctolib && grep -n "STATUS_OK" packages/@doctolib/dmp/src/business/FrDmpConstants.ts 2>/dev/null | head -5 Tool results: → result (ok): 8:export const STATUS_OK = 'OK' Tools used: - Bash: cd /Users/USER/workspace/doctolib/doctolib && grep -n "findDocumentsResponseFixtureBuilder\|s_status" packages/@doctolib/dmp/src/business/DmpApi.test.ts 2>/dev/null | head -30 Tool results: → result (ok): 25:import { documentMetadataFixtureBuilder, findDocumentsResponseFixtureBuilder } from '../test-helpers/fixtures' 184: deleteDocumentMocked.mockResolvedValue({ s_status: STATUS_OK }) 185: setConnectionConfidentialityLevelMocked.mockResolvedValue({ s_status: STATUS_OK }) 186: getDmpMajorityAgeMocked.mockResolvedValue({ s_status: STATUS_OK, i_majorityAge: 18 }) 608: setConnectionConfidentialityLevelMocked.mockResolvedValue({ s_status: STATUS_OK }) 609: getDmpMajorityAgeMoc …(truncated, 1931 more chars) Tools used: - Bash: cd /Users/USER/workspace/doctolib/doctolib && grep -n "findDocumentsResponseFixtureBuilder\|s_apiError\|s_status" packages/@doctolib/dmp/src/test-helpers/fixtures.ts 2>/dev/null | head -40 Tool results: → result (ok): 25:export const findDocumentsResponseFixtureBuilder = createFixtureBuilder({ 26: s_status: STATUS_OK, Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/test-helpers/fixtures.ts (1–51) Tool results: → result (ok): 1 import { createFixtureBuilder } from '@doctolib/test-helpers' 2 import type { DocumentMetadata, FindDocumentsResponse } from '../types/FrDmpTypes' 3 import { STATUS_OK } from '../business/FrDmpConstants' 4 5 export const documentMetadataFixtureBuilder = createFixtureBuilder({ 6 s_uniqueId: 'unique-1', 7 s_title: '', 8 s_description: '', 9 s_classCode: '', 10 s_typeCode: '', 11 s_creationDate: '', 12 s_serviceStartDate: '', 13 s_serviceStopDate: '', 14 s_hea …(truncated, 390 more chars) Tools used: - Bash: cd /Users/USER/workspace/doctolib/doctolib && grep -n "FindDocumentsResponse" packages/@doctolib/dmp/src/types/FrDmpTypes.ts Tool results: → result (ok): 411:export interface FindDocumentsResponse extends DmpResponse { Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/types/FrDmpTypes.ts (411–421) Tool results: → result (ok): 411 export interface FindDocumentsResponse extends DmpResponse { 412 / Array of document metadata matching the search criteria / 413 Documents: DocumentMetadata[] 414 / Optional error message. Consider using i_apiErrorCode and i_apiErrorType from DmpResponse. */ 415 errorMessage?: string 416 } 417 418 // Other types 419 export interface DmpWorkflow { 420 state: DmpWorkflowState Now I have a thorough picture. Let me check one final important detail — the message construction bug when apiErrorExtendedInformations is present but apiErrorDescription is undefined. Tools used: - Bash: cd /Users/USER/workspace/doctolib/doctolib && node -e " … Tool results: → result (ok): "undefined: Some timeout" That confirms an actual bug. Let me also check the toDocumentSearchResult function in DmpNhrAdapter to confirm the error field is silently dropped. Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts (108–123) Tool results: → result (ok): 108 function toDocumentSearchResult(result: DmpSearchResult): NhrDocumentSearchResult { 109 return { 110 documents: result.documents.map(toNhrDocument), 111 } 112 } 113 114 function toNhrDocumentContentResult( 115 result: DmpDocumentContent 116 ): NhrDocumentContentResult> { 117 const { component, ...metadataWithoutComponent } = result.cdaContent ?? ({} as ClinicalDocument) 118 119 let files: NhrDocume …(truncated, 131 more chars) I now have everything needed for a complete analysis. --- Summary The PR adds a typed DmpApiError class and propagates error fields from FindDocumentsResponse into SearchResult.error. The test suite is well-structured and covers the core happy/sad paths clearly. However there are meaningful gaps in coverage. --- Critical Gaps 1. Message construction bug when apiErrorExtendedInformations is set but apiErrorDescription is undefined — Criticality: 8/10 In /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/business/DmpApiError.ts (line 19–21): const message = info.apiErrorExtendedInformations ? ${info.apiErrorDescription}: ${info.apiErrorExtendedInformations} : info.apiErrorDescription ?? 'DMP API error' When apiErrorExtendedInformations is present but apiErrorDescription is absent, the message becomes the literal string "undefined: Some timeout". This is an actual bug producing a misleading error message surfaced to users and logged in monitoring. The test suite covers three branches (both present, only description, neither) but misses the fourth combination: extended info present, description absent. A test that would catch this: it('falls back to generic message when only extended info is present without description', () => { const error = new DmpApiError({ apiErrorExtendedInformations: 'Operation timed out.', status: 'ERROR' }) expect(error.message).toBe('DMP API error') // or 'Operation timed out.' depending on intended behavior }) This test would also serve as a specification: the team needs to decide whether the intended message in this case is 'DMP API error', 'Operation timed out.', or something else, and fix the implementation accordingly. 2. No test for s_status being undefined (the ?? 'ERROR' fallback branch) — Criticality: 7/10 The mapping in /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/business/DmpApi.ts (line 526): status: response.s_status ?? 'ERROR', DmpResponse.s_status is typed as string | undefined. The ?? 'ERROR' fallback fires when s_status is undefined — meaning a response with no s_status field at all is treated as an error (because undefined !== STATUS_OK). There is no test covering this scenario. If the DMP backend returns a response with no s_status, the code will set SearchResult.error with status: 'ERROR' and documents will be populated. That is a silent data-correctness issue: consumers see both a document list and an error object simultaneously. A test to add in DmpApi.test.ts: it('should set error.status to "ERROR" and still return documents when s_status is absent', async () => { const mockResponse = findDocumentsResponseFixtureBuilder({ s_status: undefined, Documents: [] }) findDocumentsMocked.mockResolvedValue(mockResponse) const result = await dmpApi.search({ ins: '1234567890' }) expect(result.error).toBeDefined() expect(result.error?.status).toBe('ERROR') }) --- Important Improvements 3. DmpNhrAdapter.toDocumentSearchResult silently drops the error field — Criticality: 6/10 In /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts, toDocumentSearchResult only maps documents and discards error. This is the primary consumer of SearchResult outside the @doctolib/dmp package. The intent of the PR is to make errors observable to callers, but they are currently invisible once the DmpNhrAdapter translates the result. While fixing this adapter is likely out of scope for this PR, a test in DmpNhrAdapter.test.ts asserting that the error field is dropped (or propagated) would document the current contract and prevent a silent behavioral regression if someone adds error propagation later: it('should return search result without error field when dmpApi returns an error', async () => { const mockDmpApi = createMockDmpApi({ documents: [], error: { status: 'ERROR', apiErrorDescription: 'Timed out' }, }) const result = await new DmpNhrAdapter(mockDmpApi).searchDocuments(searchProps) expect(result).not.toHaveProperty('error') }) 4. No test verifying documents are still populated when an error is present — Criticality: 6/10 The PR's tests on the error path always use Documents: []. The toSearchResult logic is capable of returning both non-empty documents and an error simultaneously (the mapping runs independently). While this may be intentional, it is untested and could cause a UI regression if consumers assume documents is always empty when error is set. A test with documents present on an error response would either confirm this is the intended contract or expose an undesired coupling. 5. No test for DmpApiError stack trace preservation — Criticality: 5/10 DmpApiError extends Error and sets this.name in the constructor. In environments using super(), there are known TypeScript/V8 prototype chain issues unless Object.setPrototypeOf(this, DmpApiError.prototype) is called. The existing instanceof Error test passes trivially (the prototype chain works in the Jest/Node environment). A test for error instanceof DmpApiError would be more meaningful and would catch a prototype chain regression: it('is an instance of DmpApiError itself', () => { const error = new DmpApiError({ status: 'ERROR' }) expect(error).toBeInstanceOf(DmpApiError) }) --- Test Quality Issues The "should not throw" test is redundant and weak. The test at line 1051 in DmpApi.test.ts (resolves.toBeDefined()) verifies no throw, but resolves.toBeDefined() would pass even if the promise resolved to null. The scenario it covers (ERROR status with a description) is already covered by the fuller assertion in the preceding test. This test does not add meaningful regression protection. The field mapping test in DmpApiError.test.ts uses expect.toEqual indirectly via individual assertions, but the message construction test does not cover all four branches of the conditional logic. The tests are otherwise well-scoped and follow DAMP principles with literal values rather than shared constants. --- Positive Observations - The three DmpApiError message assembly tests are well-named and directly test the branching logic, not the constructor's internal implementation. - The error identity tests (instanceof Error, name === 'DmpApiError') are valuable for consumers using catch (e) blocks with type narrowing. - The DmpApi.test.ts tests correctly use toEqual with an explicit shape rather than toMatchObject, ensuring no unexpected extra fields are present in SearchResult.error. - Using findDocumentsResponseFixtureBuilder keeps the test setup minimal and reduces maintenance overhead as the fixture evolves. - New fields in FrDmpTypes.ts (s_apiErrorContext, s_apiErrorDescription, s_apiErrorExtendedInformations, s_apiErrorTypeDescription) are correctly typed as optional on DmpResponse, consistent with the existing pattern for i_apiErrorCode and i_apiErrorType.