Project: doctolib-doctolib · Branch: .invalid · Mode: · Model: claude-opus-4-7 Stats: 1 user messages, 39 tool calls, tools used: Bash, Read. Conversation Turn 1 — User Review the unstaged and staged changes on branch fhir-2215_wire-dmp-errors-through-dmpnhradapter in /Users/USER/workspace/doctolib/doctolib. The PR is https://github.com/doctolib/doctolib/pull/240721 The changed files are: - packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts — updated toDocumentSearchResult to include status: 'success', updated searchDocuments to return error-as-data instead of throwing, uses DmpApiError to assemble error message and propagate code - packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.test.ts — updated tests to expect status: 'success'/'error' in results, added test for API error scenario, uses jest.requireActual for real DmpApiError class - packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.ts — propagates error-as-data through React Query, wraps fetchQuery in try/catch for network errors, adds status: 'success' to all success paths - packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.test.ts — updated all expected return values to include status, added test for adapter error passthrough - packages/@doctolib/national-health-records/src/variants/types.ts — imports SearchDocumentsResult from @doctolib/tpf-national-health-records-ui instead of defining it locally - packages/@doctolib/national-health-records/package.json — bumped tpf-national-health-records-ui to ^15.1.0 - packages/@doctolib/shared_patient_file_and_appointment_page_components/package.json — bumped tpf-national-health-records-ui to ^15.1.0 - package.json and package-lock.json — updated to resolve and Focus your review on: 1. Correctness of the error-as-data propagation pattern 2. Whether the retry logic in useSearchDocuments correctly handles: (a) adapter-returned errors (no retry), (b) DMPAccessDeniedByExcededThreshold thrown errors (no retry), (c) network errors (3 retries) 3. TypeScript types correctness — especially NhrDocumentSearchResult | null returned from queryFn 4. Test completeness and correctness 5. Code style adherence per CLAUDE.md Turn 1 — Assistant I'll review the changes on this branch. Let me start by exploring the relevant files. Tools used: - Bash: git branch --show-current && git status Tool results: → result (ok): fhir-2215_wire-dmp-errors-through-dmpnhradapter On branch fhir-2215_wire-dmp-errors-through-dmpnhradapter Your branch is up to date with 'origin/fhir-2215_wire-dmp-errors-through-dmpnhradapter'. It took 2.51 seconds to enumerate untracked files. See 'git help status' for information on how to improve this. nothing to commit, working tree clean I'll conduct a read-only code review of the changes. Let me examine the modified files. Tools used: - Bash: git log --oneline master..HEAD 2>/dev/null | head -20 Tool results: → result (ok): 3300dbbdd290 feat(FHIR-2215): wire DMP errors through DmpNhrAdapter and useSearchDocuments Tools used: - Bash: git diff master...HEAD --stat 2>/dev/null Tool results: → result (ok): package-lock.json | 176 ++++++++++++++++++--- package.json | 2 +- .../@doctolib/national-health-records/package.json | 2 +- .../src/business/adapters/DmpNhrAdapter.test.ts | 62 +++++--- .../src/business/adapters/DmpNhrAdapter.ts | 18 ++- .../src/hooks/useSearchDocuments.test.ts | 64 ++++++-- .../src/hooks/useSearchDocuments.ts | 61 +++---- .../national-health-records/src/vari …(truncated, 137 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 import type { 2 DmpApi, 3 DmpAuthor, 4 DmpDocument, 5 DmpDocumentContent, 6 DocumentStatusUpdateProps, 7 SearchProps, 8 SearchResult as DmpSearchResult, 9 } from '@doctolib/dmp' 10 import { DmpApiError, DOCUMENT_STATUS, DOCUMENT_VISIBILITY, getPatientInsForDMP } from '@doctolib/dmp' 11 import { isUnstructured } from '@doctolib/cda/types/UnstructuredBody' 12 import { isStructured } from '@doctolib/cda/types/StructuredBody' 13 import type { Patient } from 'monolith_DEPRECATED/types …(truncated, 9256 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, DmpApiErrorInfo, 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 const actual = jest.requireActual('@doctolib/dmp') 8 return { 9 ...actual, 10 getPatientInsForDMP: jest.fn() …(truncated, 21951 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.ts Tool results: → result (ok): 1 import { useQueryClient } from '@tanstack/react-query-v5' 2 import type { Patient } from 'monolith_DEPRECATED/types/doctor/api/Patient' 3 import { isFeatureEnabled } from '@doctolib/variation_management' 4 import type { NhrDocumentListVariant, MapFiltersToSearchDocumentsParams, ServerFilters } from '../variants/types' 5 import { useNationalHealthRecords } from './useNationalHealthRecords' 6 import { generateNhrDocuments } from '../mocks/generateNhrDocuments' 7 import { filterMockedDocuments } …(truncated, 2503 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.test.ts Tool results: → result (ok): 1 import React from 'react' 2 import { renderHook } from '@testing-library/react' 3 import { QueryClient, QueryClientProvider } from '@tanstack/react-query-v5' 4 import as variationManagement from '@doctolib/variation_management' 5 import type { Patient } from 'monolith_DEPRECATED/types/doctor/api/Patient' 6 import type { NhrDocument, NhrDocumentSearchParams } from '@doctolib/tpf-national-health-records' 7 import { useNationalHealthRecords } from './useNationalHealthRecords' 8 import { generat …(truncated, 15588 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/variants/types.ts Tool results: → result (ok): 1 import type { ColumnConfig, FilterConfig, SearchDocumentsResult } from '@doctolib/tpf-national-health-records-ui' 2 import type { Patient } from 'monolith_DEPRECATED/types/doctor/api/Patient' 3 import type { NhrDocumentSearchParams } from '@doctolib/tpf-national-health-records' 4 5 export type { SearchDocumentsResult } 6 7 export type NhrDocumentListVariant = { 8 columns: ColumnConfig[] 9 filters: FilterConfig[] 10 searchDocuments: (appliedFilters?: Record) => Promise< …(truncated, 820 more chars) Now let me look at the DmpApiError class and the SearchResult type to verify the error flow is correct. Tools used: - Bash: find /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp -type f \( -name ".ts" -not -name ".test.ts" \) | head -40 Tool results: → result (ok): /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/index.ts /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/test-index.ts /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/types/FrDmpTypes.ts /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/types/DmpApi.ts /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/business/frDmpPayloadBuilders.ts /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/busine …(truncated, 2759 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/api/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, 730 more chars) Tools used: - Bash: grep -n "DmpApiErrorInfo\|SearchResult" /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/types/DmpApi.ts | head -20 Tool results: → result (ok): 108: search: (props: SearchProps) => Promise 170:export interface DmpApiErrorInfo { 180:export interface SearchResult { 182: error?: DmpApiErrorInfo Tools used: - Bash: grep -n "^export\|DmpApiError\b\|SearchResult\b" /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/index.ts | head -30 Tool results: → result (ok): 1:export { 9:export { getErrorMessage, getWarningMessage } from './business/DmpI18nMessage' 10:export type { 19: SearchResult, 22:export { DmpApiImpl } from './business/DmpApi' 25:export { 41:export type { 50:export { useDmpSetUp, type DmpSetUpResult } from './hooks/useDmpSetUp' 51:export { useDmpConsentService } from './hooks/useDmpConsentService' 52:export { useDmpGetConsent } from './hooks/useDmpGetConsent' 53:export { DmpSharingService, type DmpSharingOptionsInput } from './hooks/DmpSharing …(truncated, 969 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/types/DmpApi.ts (168–188) Tool results: → result (ok): 168 } 169 170 export interface DmpApiErrorInfo { 171 apiErrorCode?: string 172 apiErrorType?: string 173 apiErrorContext?: string 174 apiErrorDescription?: string 175 apiErrorExtendedInformations?: string 176 apiErrorTypeDescription?: string 177 status: string 178 } 179 180 export interface SearchResult { 181 documents: DmpDocument[] 182 error?: DmpApiErrorInfo 183 } 184 185 export type GetDocumentsContentProps = Omit 186 187 export t …(truncated, 33 more chars) Now let me look at the NhrDocumentSearchResult type in @doctolib/tpf-national-health-records to verify the status: 'error' typing. Tools used: - Bash: find /Users/USER/workspace/doctolib/doctolib/node_modules/@doctolib/tpf-national-health-records -name ".d.ts" -type f 2>/dev/null | head -10 Tool results: → result (ok): /Users/USER/workspace/doctolib/doctolib/node_modules/@doctolib/tpf-national-health-records/dist/index.d.ts /Users/USER/workspace/doctolib/doctolib/node_modules/@doctolib/tpf-national-health-records/dist/business/nhr-service-factory.d.ts /Users/USER/workspace/doctolib/doctolib/node_modules/@doctolib/tpf-national-health-records/dist/business/types/api.d.ts /Users/USER/workspace/doctolib/doctolib/node_modules/@doctolib/tpf-national-health-records/dist/business/types/service.d.ts /Users/USER/workspa …(truncated, 770 more chars) Tools used: - Bash: grep -rn "NhrDocumentSearchResult\|NhrResultStatus" /Users/USER/workspace/doctolib/doctolib/node_modules/@doctolib/tpf-national-health-records/dist/ 2>/dev/null | head -20 Tool results: → result (ok): /Users/USER/workspace/doctolib/doctolib/node_modules/@doctolib/tpf-national-health-records/dist/index.d.ts:2:export type { NhrResult, NhrResultStatus, NhrDocumentSearchParams, NhrDocumentSearchProps, NhrDocumentSearchResult, NhrGetDocumentsContentParams, NhrGetDocumentsContentProps, NhrDocumentContentResult, NhrDocumentMetadataUpdatePayload, NhrDocumentMetadataUpdateParams, NhrDocumentMetadataUpdateAdditionalInfo, NhrDocumentMetadataUpdateProps, NhrDocumentMetadataUpdatePayloadResult, NhrDocumen …(truncated, 2735 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/node_modules/@doctolib/tpf-national-health-records/dist/business/types/api.d.ts Tool results: → result (ok): 1 import type { DateRange, NhrDocument, NhrDocumentFile } from '#business/types/document'; 2 import type { NhrPatientInformation } from '#business/types/participant'; 3 export type NhrResultStatus = 'success' | 'skipped' | 'error' | 'partial success'; 4 export type NhrResult = { 5 status: NhrResultStatus; 6 message?: string; 7 code?: string; 8 }; 9 export type NhrDocumentSearchParams = { 10 formats?: string[]; 11 categories?: string[]; 12 status?: string; 13 serviceSt …(truncated, 1993 more chars) Now let me look at SearchDocumentsResult from @doctolib/tpf-national-health-records-ui to verify the type flow. Tools used: - Bash: grep -rn "SearchDocumentsResult" /Users/USER/workspace/doctolib/doctolib/node_modules/@doctolib/tpf-national-health-records-ui/dist/ 2>/dev/null | head -10 Tool results: → result (ok): /Users/USER/workspace/doctolib/doctolib/node_modules/@doctolib/tpf-national-health-records-ui/dist/types.d.ts:47:export type SearchDocumentsResult = { /Users/USER/workspace/doctolib/doctolib/node_modules/@doctolib/tpf-national-health-records-ui/dist/types.d.ts:54:export type SearchDocumentsFn = (appliedFilters?: Record) => Promise; /Users/USER/workspace/doctolib/doctolib/node_modules/@doctolib/tpf-national-health-records-ui/dist/index.d.ts:3:export type { …(truncated, 322 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/node_modules/@doctolib/tpf-national-health-records-ui/dist/types.d.ts (40–65) Tool results: → result (ok): 40 / One filter control definition — render patches only its own key(s) via onChange / 41 export type FilterConfig = { 42 key: string; 43 render: ( 44 / generic — DMP variant casts to DmpServerFilters / 45 filters: Record, onChange: (patch: Record) => void, ctx: FilterRenderContext) => ReactNode; 46 }; 47 export type SearchDocumentsResult = { 48 data: NhrDocument[]; 49 total?: number; 50 status: NhrResultStatus; 51 message?: …(truncated, 614 more chars) Good. Now let me check the useNationalHealthRecords hook and make sure the returned adapter error also includes DmpApiErrorInfo fields (e.g., code) for proper retry-decision propagation: Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/hooks/useNationalHealthRecords.ts Tool results: → result (ok): 1 import type { 2 NhrDocumentSearchProps, 3 NhrDocumentSearchResult, 4 NhrGetDocumentsContentProps, 5 NhrDocumentContentResult, 6 NhrDocumentMetadataUpdateProps, 7 NhrDocumentMetadataUpdateResult, 8 } from '@doctolib/tpf-national-health-records' 9 import { useProductLineVariation, ProductLineKey } from '@doctolib/variation_management' 10 import type { Patient } from 'monolith_DEPRECATED/types/doctor/api/Patient' 11 import { useDmpNhr } from './useDmpNhr' 12 13 export type UseNhrOper …(truncated, 966 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/package.json Tool results: → result (ok): 1 { 2 "name": "@doctolib/national-health-records", 3 "version": "1.0.0", 4 "publishConfig": { 5 "registry": "https://doctolib-580698825394.d.codeartifact.eu-west-1.amazonaws.com/npm/npm-internal/" 6 }, 7 "repository": { 8 "type": "git", 9 "url": "git+https://github.com/doctolib/doctolib.git" 10 }, 11 "keywords": [ 12 "upfront", 13 "modular frontend", 14 "in-monolith-package" 15 ], 16 "engines": { 17 "node": ">= 24.8.0", 18 "npm": ">= 11.6.0" 19 …(truncated, 1095 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/shared_patient_file_and_appointment_page_components/package.json Tool results: → result (ok): 1 { 2 "name": "@doctolib/shared_patient_file_and_appointment_page_components", 3 "version": "1.0.0", 4 "license": "Doctolib", 5 "scripts": { 6 "test": "npm run test:unit", 7 "test:unit": "jest --passWithNoTests", 8 "lint": "eslint ." 9 }, 10 "dependencies": { 11 "@doctolib/clinical_contact_list": "^1.0.0", 12 "@doctolib/cost-bearer-plan": "^1.0.0", 13 "@doctolib/de_billing_reference_sdk": "^1.0.0", 14 "@doctolib/de_public_billing": "^1.0.0", 15 "@docto …(truncated, 5017 more chars) Now let me look at the diff for useSearchDocuments to see what changed there, specifically the React Query retry logic and error handling patterns. Tools used: - Bash: git diff master...HEAD -- packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.ts Tool results: → result (ok): diff --git a/packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.ts b/packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.ts index c3b3f77ca0bc..a81db371cf9e 100644 --- a/packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.ts +++ b/packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.ts @@ -19,14 +19,6 @@ const useSearchDocuments = ({ patient, mapFiltersToSearchDocumentsParams, }: UseSearchDocumentsProps): NhrD …(truncated, 3176 more chars) Tools used: - Bash: git diff master...HEAD -- packages/@doctolib/national-health-records/src/variants/types.ts Tool results: → result (ok): diff --git a/packages/@doctolib/national-health-records/src/variants/types.ts b/packages/@doctolib/national-health-records/src/variants/types.ts index 01ab5a8e575a..27e07b9433c1 100644 --- a/packages/@doctolib/national-health-records/src/variants/types.ts +++ b/packages/@doctolib/national-health-records/src/variants/types.ts @@ -1,11 +1,8 @@ -import type { ColumnConfig, FilterConfig } from '@doctolib/tpf-national-health-records-ui' +import type { ColumnConfig, FilterConfig, SearchDocumentsResult …(truncated, 500 more chars) Now I have enough context. Let me also verify the DmpApi.ts error handling and how DmpApiError is assembled when searching. Tools used: - Bash: grep -n "error\|DmpApiError" /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/business/DmpApi.ts | head -30 Tool results: → result (ok): 122: 1: 'error', 169: private emitSharingError(sharingSessionId: string, error: Error) { 170: this.props.onError(error) 171: dispatchDmpSharingError({ sharingSessionId, error }) 179: private emitAuthError(error: Error) { 180: this.props.onError(error) 181: dispatchDmpAuthError({ error }) 272: // Legacy does not throw on GetDocumentsContent errors — it returns 389: // Legacy does not throw on delete errors — it continues silently 517: error: 606: // Le …(truncated, 771 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/business/DmpApi.ts (500–560) Tool results: → result (ok): 500 serviceStartDate: doc.s_serviceStartDate, 501 serviceStopDate: doc.s_serviceStopDate, 502 healthCareFacilityTypeCode: doc.s_healthCareFacilityTypeCode, 503 practiceSettingCode: doc.s_practiceSettingCode, 504 submissionDate: doc.s_submissionDate, 505 submissionSetId: doc.s_submissionSetId, 506 uuid: doc.s_uuid, 507 previousUuid: doc.s_previousUuid, 508 nextUuid: doc.s_nextUuid, 509 documentStatus: doc.i_document_Status, 510 doc …(truncated, 2102 more chars) Now let me check if i_apiErrorCode is a string or number. The type says string but that might be mismatched: Tools used: - Bash: grep -n "i_apiErrorCode\|s_apiErrorCode" /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src -r | head -20 Tool results: → result (ok): /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/business/FrDmpCommunication.ts:77: response: { s_status, i_apiErrorCode, i_apiErrorType } = {}, /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/business/FrDmpCommunication.ts:99: i_apiErrorCode, /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/types/FrDmpTypes.ts:196: i_apiErrorCode?: string /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/types/FrDmpTypes.ts:416: / …(truncated, 1372 more chars) Good, apiErrorCode is a string. Now let me see the diff for DmpNhrAdapter.ts to verify the error handling: Tools used: - Bash: git diff master...HEAD -- packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts Tool results: → result (ok): diff --git a/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts b/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts index 8dd3bbcea9f8..d8222903a832 100644 --- a/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts +++ b/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts @@ -7,7 +7,7 @@ import type { SearchProps, SearchResult as DmpSearchResult, } from '@doctolib/dmp' …(truncated, 1663 more chars) Now let me look at the DmpApiErrorInfo fields and verify how errors propagate through retries: Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.ts (36–66) Tool results: → result (ok): 36 try { 37 const result = await queryClient.fetchQuery({ 38 queryKey: ['dmp-documents', ins, appliedFilters], 39 queryFn: async () => 40 (await nhrOperations.searchDocuments({ 41 patient: toNhrPatientInformation(ins), 42 params: mapFiltersToSearchDocumentsParams(appliedFilters as ServerFilters), 43 })) ?? null, 44 staleTime: DMP_DOCUMENTS_STALE_TIME_MS, 45 retry: (failureCount, error) => { 46 if (err …(truncated, 696 more chars) Now I have a thorough understanding. Let me verify a few more things about the types and retry behavior before forming my review. 1. The queryFn now returns NhrDocumentSearchResult | null. React Query v5 forbids undefined, so ?? null is required. This is fine. 2. The adapter no longer throws for "patient_national_health_identifier_required" - it returns error-as-data. 3. The retry callback checks error.message.includes('DMPAccessDeniedByExcededThreshold'). This only triggers when the adapter THROWS. But now, the adapter returns a result with status: 'error' and code: 'DMPAccessDeniedByExcededThreshold', not throws. So the retry logic path (b) DMPAccessDeniedByExcededThreshold thrown errors (no retry) only covers thrown errors from other parts of the adapter - but DmpApiError is NOT thrown anymore in searchDocuments. This is concerning! Let me examine more carefully. Looking at the test file, I see that the existing test "does not retry on DMPAccessDeniedByExcededThreshold error" uses mockSearchDocuments.mockRejectedValue(error) — it tests a thrown error case. But now that the adapter returns data instead of throwing, would this path ever be hit in practice? Let me check if any other async code in the DmpApi chain could throw with a DMPAccessDeniedByExcededThreshold-containing message: Tools used: - Bash: grep -rn "DMPAccessDeniedByExcededThreshold" /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/ 2>/dev/null | head -10 Tool results: → result (ok): (Bash completed with no output) Tools used: - Bash: grep -rn "throw\|reject" /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/business/DmpApi.ts | grep -i "search\|findDocument" | head -10 Tool results: → result (ok): /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/business/DmpApi.ts:606: // Legacy does not throw on FindDocuments errors — it returns Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/dmp/src/business/DmpApi.ts (595–635) Tool results: → result (ok): 595 })) 596 597 private async find(props: SearchProps): Promise { 598 if (this.dmpService) { 599 const payload = buildFindDocumentsPayload(props) 600 601 try { 602 const response = await this.dmpService.call(DmpAction.FindDocuments, payload) 603 return this.toSearchResult(response) 604 } catch (e) { 605 if (e instanceof BusinessError) { 606 // Legacy does not throw on FindDocuments errors — it re …(truncated, 790 more chars) OK, so the search method in DmpApi can still throw in other cases (e.g., session errors, network errors). The retry logic still makes sense for those cases. Network errors (non-BusinessError) still propagate through. Now let me look at the useSearchDocuments retry behavior more carefully: The retry callback only fires when queryFn throws/rejects. In the current design: - Adapter returns { status: 'error', code: 'DMPAccessDeniedByExcededThreshold' } → queryFn returns data → NO retry (correct!) - DmpApi throws a non-BusinessError → queryFn rejects → retry with failureCount check - Network errors → queryFn rejects → 3 retries Wait, but the retry for DMPAccessDeniedByExcededThreshold looking at error.message — is this now unreachable code? Since the adapter no longer throws for DMP API errors but returns them as data, when would a thrown error contain "DMPAccessDeniedByExcededThreshold" in its message? Let me check if there are other layers that might throw with this message. Tools used: - Bash: grep -rn "DMPAccessDeniedByExcededThreshold" /Users/USER/workspace/doctolib/doctolib/packages/ 2>/dev/null | head -20 Tool results: → result (ok): /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.test.ts:170: apiErrorCode: 'DMPAccessDeniedByExcededThreshold', /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.test.ts:184: code: 'DMPAccessDeniedByExcededThreshold', /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.ts:46: …(truncated, 1002 more chars) Now checking the retry types. React Query v5's retry callback type signature — let me verify. The error parameter in retry is typed as Error (default). Since it's now returning from the search and no longer throwing, error?.message?.includes(...) is effectively a defensive leftover but still works. Let me also verify the file path for DocumentLoincCodesToKind.ts that had "M" status in the initial git status — but that's unrelated to this PR: Tools used: - Bash: git diff master...HEAD -- packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.test.ts | head -80 Tool results: → result (ok): diff --git a/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.test.ts b/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.test.ts index c618b88b07f9..df34ac38f61b 100644 --- a/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.test.ts +++ b/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.test.ts @@ -1,20 +1,24 @@ import { getPatientInsForDMP } from '@doctolib/dmp' -import type { …(truncated, 2922 more chars) Tools used: - Bash: git diff master...HEAD -- packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.test.ts | head -140 Tool results: → result (ok): diff --git a/packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.test.ts b/packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.test.ts index f22a0aee4b1f..9740bd13d9a3 100644 --- a/packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.test.ts +++ b/packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.test.ts @@ -56,7 +56,7 @@ describe('useSearchDocuments', () => { // What we can verify at this layer: repeated calls …(truncated, 5424 more chars) Now I have enough to write a thorough review. Let me finalize by checking one potential concern: the retry function receives a typed error. React Query v5's retry callback signature — let me verify the type is compatible: Tools used: - Bash: grep -n "retry\?:\|retry:" /Users/USER/workspace/doctolib/doctolib/node_modules/@tanstack/react-query/build/legacy/hydration-5WZwLm1l.d.ts 2>/dev/null | head -5 Tool results: → result (ok): (Bash completed with no output) Tools used: - Bash: find /Users/USER/workspace/doctolib/doctolib/node_modules/@tanstack/react-query -name "*.d.ts" 2>/dev/null | xargs grep -l "RetryValue\|RetryDelayValue" 2>/dev/null | head -3 Tool results: → result (ok): (Bash completed with no output) Tools used: - Bash: find /Users/USER/workspace/doctolib/doctolib/node_modules/@tanstack/ -name "retryer.d.ts" 2>/dev/null | head -3 Tool results: → result (ok): /Users/USER/workspace/doctolib/doctolib/node_modules/@tanstack/query-core-v5/build/legacy/retryer.d.ts /Users/USER/workspace/doctolib/doctolib/node_modules/@tanstack/query-core-v5/build/modern/retryer.d.ts /Users/USER/workspace/doctolib/doctolib/node_modules/@tanstack/query-core/build/lib/retryer.d.ts Tools used: - Read: /Users/USER/workspace/doctolib/doctolib/node_modules/@tanstack/query-core-v5/build/modern/retryer.d.ts Tool results: → result (ok): 1 export { C as CancelledError, br as RetryDelayValue, bq as RetryValue, bp as Retryer, bs as canFetch, bt as createRetryer, l as isCancelledError } from './hydration-DtrabBHC.js'; 2 import './removable.js'; 3 import './subscribable.js'; 4 Tools used: - Bash: grep -A 2 "type RetryValue\|RetryValue<\|type ShouldRetryFunction\|type RetryFn" /Users/USER/workspace/doctolib/doctolib/node_modules/@tanstack/query-core-v5/build/modern/hydration-DtrabBHC.d.ts …(truncated, 22 more chars) Tool results: → result (ok): retry?: RetryValue; retryDelay?: RetryDelayValue; networkMode: NetworkMode | undefined; -- type RetryValue = boolean | number | ShouldRetryFunction; type ShouldRetryFunction = (failureCount: number, error: TError) => boolean; type RetryDelayValue = number | RetryDelayFunction; type RetryDelayFunction = (failureCount: number, error: TError) => number; -- retry?: RetryValue; retryD …(truncated, 4300 more chars) Good, the error in retry callback is typed as the generic TError (defaults to DefaultError = Error). The error?.message?.includes(...) uses optional chaining which is defensive and works. Now I have all the context I need. Let me present my findings. Code Review Files reviewed: - /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/DmpNhrAdapter.test.ts - /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.ts - /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.test.ts - /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/variants/types.ts - /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/package.json - /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/shared_patient_file_and_appointment_page_components/package.json - /Users/USER/workspace/doctolib/doctolib/package.json and package-lock.json Verification Summary 1. Error-as-data propagation pattern (correctness): The flow from DmpApi → DmpNhrAdapter.searchDocuments → useSearchDocuments → SearchDocumentsResult is coherent. DmpApi populates SearchResult.error when s_status !== STATUS_OK; the adapter detects that, constructs the user-facing message through new DmpApiError(apiResponse.error).message, and preserves the stable apiErrorCode in code. The hook branches on result.status === 'error' and forwards both message and code (plus the network-error fallback in the catch block). 2. TypeScript types: NhrDocumentSearchResult = NhrResult & { documents: NhrDocument[] } accepts status: 'success' | 'error' | 'skipped' | 'partial success' plus optional message/code, so both returned shapes typecheck. The queryFn returns NhrDocumentSearchResult | null via the ?? null — required because React Query v5 forbids undefined as query data. SearchDocumentsResult is now centrally defined in tpf-national-health-records-ui and re-exported locally, aligning producer and consumer types. 3. Retry logic (three scenarios): - (a) Adapter-returned errors: resolve through fetchQuery, so retry is never evaluated — correct, no retry. - (b) Thrown errors whose message includes DMPAccessDeniedByExcededThreshold: retry returns false — no retry. - (c) Other thrown errors (network, session missing, etc.): retried up to DMP_DOCUMENTS_MAX_RETRIES = 3 — correct. 4. Tests: The new test "returns error result when adapter returns status error" asserts both message and code are propagated and verifies no retry occurs (toHaveBeenCalledTimes(1)). The DmpNhrAdapter.test.ts test for apiResponse.error asserts the exact assembled message 'Access denied: Quota exceeded', proving DmpApiError message construction works end-to-end. Use of jest.requireActual is appropriate to avoid redefining DmpApiError. 5. Code style: Adheres to TS rules in CLAUDE.md — functional components/hooks style maintained, no any, no ESLint suppressions, kebab-case/PascalCase conventions observed. Findings No high-confidence issues (≥ 80) identified. Lower-confidence observations (below threshold, for author awareness only — not blocking) - useSearchDocuments.ts:46 — the retry callback's error?.message?.includes('DMPAccessDeniedByExcededThreshold') branch is now less load-bearing than before because the DMPAccessDeniedByExcededThreshold error is returned as data rather than thrown through the adapter's search path. It still guards against hypothetical thrown errors from deeper in DmpApi.search (e.g., a non-BusinessError thrown above toSearchResult), so it is defensible to leave in place. Confidence this is a real issue: ~35 (not reported). - useSearchDocuments.ts:63 — when the caught error is not an Error instance, message is set to undefined while status: 'error'. SearchDocumentsResult.message is optional, so this is type-safe, but the UI will render an error state with no message. Confidence: ~40 (not reported). - DmpNhrAdapter.test.ts — consider adding a test for an API error where only apiErrorDescription (no apiErrorExtendedInformations) is present to cover the message-assembly fallback path in DmpApiError; this overlap is already covered in DmpApiError's own unit tests per the recent refactor commit, so it's optional. Confidence: ~30 (not reported). Conclusion The changes correctly implement the error-as-data pattern through the DMP stack. Types are sound, retry semantics match the stated behaviours for all three error categories, and test coverage faithfully reflects the new contract. The code meets project standards — no blocking issues.