Project: workspace-doctolib · Branch: HEAD · Mode: · Model: claude-opus-4-7 Stats: 1 user messages, 23 tool calls, tools used: Bash, Read. Conversation Turn 1 — User Review the following GitHub PR for code that can be simplified for clarity, consistency, and maintainability: PR URL: https://github.com/doctolib/doctolib/pull/241133 Repo: doctolib/doctolib Use the GitHub MCP tools to read the PR diff. If you need to browse local code for context, the local worktree is at /Users/USER/workspace/doctolib/worktrees/doctolib/ (check subdirectories for the relevant branch). Focus only on the changed code. Look for: - Overly complex logic that can be simplified - Redundant code - Opportunities to use existing utilities/helpers - Verbose code that can be expressed more concisely Return a structured list of suggestions with file paths and line numbers. Do NOT make any actual code changes — analysis only. Turn 1 — Assistant I'll analyze the PR for simplification opportunities. Let me fetch the PR details first. Tools used: - Bash: gh pr view 241133 --repo doctolib/doctolib --json title,body,files,headRefName,baseRefName,additions,deletions Tool results: → result (ok): {"additions":228,"baseRefName":"master","body":"## Why\n\nhttps://doctolib.atlassian.net/browse/FHIR-2224\n\nupdateDocumentStatus in DmpApi previously returned Promise and threw on failure. Callers had no way to access structured DMP error information (error codes, descriptions) — they could only catch a plain Error with a message string. This PR makes the API return structured results, consistent with how search() returns SearchResult with an optional error: DmpApiErrorInfo. …(truncated, 3566 more chars) Let me fetch the actual diff for the changed files. Tools used: - Bash: gh pr diff 241133 --repo doctolib/doctolib 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 2b146a28faf65..99769412b8646 100644 --- a/packages/@doctolib/dmp/src/business/DmpApi.test.ts +++ b/packages/@doctolib/dmp/src/business/DmpApi.test.ts @@ -1609,33 +1609,78 @@ describe('DmpApi', () => { }) }) - it('should throw when no cached session exists', async () => { + it('should return success: true on success', async () => { + getCachedDmpSessi …(truncated, 18145 more chars) Now let me look at the actual file content for context, particularly to see the surrounding helpers and patterns used elsewhere. Tools used: - Bash: find /Users/USER/workspace/doctolib/worktrees/doctolib -maxdepth 1 -type d 2>/dev/null | head -20 Tool results: → result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2221_display-last-dmp-access-date /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2223_fix-column-widths-dmp /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2209_fix-patient-ins-env-transformation /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2190_improve-filter-panel-ui /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2089 /Users/USER/workspace/doctolib/worktrees …(truncated, 585 more chars) Tools used: - Bash: ls /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/packages/@doctolib/dmp/src/business/ /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/packages/@doctolib/dmp/src/typ …(truncated, 150 more chars) Tool results: → result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/packages/@doctolib/national-health-records/src/business/adapters/: DmpNhrAdapter.test.ts DmpNhrAdapter.ts parseHl7DateToIso.test.ts parseHl7DateToIso.ts /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/packages/@doctolib/dmp/src/business/: CallDmpDelegate.test.ts CallDmpDelegate.ts DmpApi.test.ts DmpApi.ts DmpI18nMessage.ts DmpSharingSettings.test.ts DmpSharingSettings.ts FrDmpCommunication.test.ts FrDmpCommunication.ts FrDm …(truncated, 230 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/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, PracticeLocationOverride } from './FrDmpTypes' 5 6 export type PatientConsent = 'error' | 'authorized' | 'expired' | 'forbidden' | 'noAuthorization' 7 8 export type PatientConsentU …(truncated, 5382 more chars) Now let me look at the actual implementation file to understand the context fully. Tools used: - Read: /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/packages/@doctolib/dmp/src/business/DmpApi.ts (340–420) Tool results: → result (ok): 340 341 const { sessionId } = cachedSession 342 const response = await getDocumentsContent({ sessionId, ...props }) 343 return response.Documents.map(this.toDocumentContent) 344 } 345 346 async updateDocumentStatus(props: DocumentStatusUpdateProps): Promise { 347 const { ins, documentUniqueId: uniqueId, uuid, healthCareSettings, getTransactionFrames } = props 348 349 if (this.dmpService) { 350 const payload = buildUpdateDocumentStatusP …(truncated, 2336 more chars) Wait - the diff shows two different shapes. Let me look more carefully. The PR description says DocumentStatusUpdateResult is { error?: DmpApiErrorInfo }, but the diff for types/DmpApi.ts shows a discriminated union { success: true } | { success: false; error: DmpApiErrorInfo }. And the actual current file at HEAD shows { error?: DmpApiErrorInfo }. Let me check the full DmpApi.ts and DmpNhrAdapter.ts current state. Tools used: - Bash: cd /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224 && git log --oneline -5 && git diff HEAD~1 HEAD -- packages/@doctolib/dmp/src/types/DmpApi.ts 2>&1 | head -30 Tool results: → result (ok): 1cec77f7f3d2 FHIR-2224: Handle errors fatal: bad revision 'HEAD~1' Tools used: - Read: /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/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 { DmpApiError, 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 t …(truncated, 9948 more chars) I notice the local file differs from the PR diff - there are two different versions. Let me check git status to understand what's the latest diff for the actual PR. Tools used: - Bash: cd /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224 && git log origin/master..HEAD --oneline 2>&1 | head -20 && echo "---" && git status -uno 2>&1 | head -10 Tool results: → result (ok): 1cec77f7f3d2 FHIR-2224: Handle errors --- Not currently on any branch. nothing to commit (use -u to show untracked files) Tools used: - Bash: cd /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224 && git log --oneline HEAD -3 2>&1 | head -10 && echo "---" && git rev-parse HEAD Tool results: → result (ok): 1cec77f7f3d2 FHIR-2224: Handle errors --- 1cec77f7f3d288e2d81bcd914047e66c14d28b3c Tools used: - Bash: gh pr view 241133 --repo doctolib/doctolib --json commits --jq '.commits | .[] | {oid: .oid, message: .messageHeadline}' Tool results: → result (ok): {"message":"FHIR-2224: Return errors for doc status updates from DMP Api","oid":"97d7243956ef7c0d5bf2b29feaccff2452a48d14"} {"message":"FHIR-2224: Handle errors","oid":"1cec77f7f3d288e2d81bcd914047e66c14d28b3c"} {"message":"FHIR-2224: address PR review - discriminated union + full error-as-data","oid":"fca4d9567bf1daf58cbb9f15e24528dec674ef7a"} {"message":"FHIR-2224: make updateDocumentStatus fully error-as-data","oid":"de54a2921ee1e701ef5ff8bedd445b582351aed2"} {"message":"Merge branch 'master' …(truncated, 192 more chars) So the PR has been updated with newer commits. The full diff I fetched is the up-to-date diff. The local worktree is stale (only has the first commit). The latest PR diff uses the discriminated union { success: true } | { success: false; error: DmpApiErrorInfo }. Let me fetch the latest content of each modified file from the PR. Tools used: - Bash: gh api repos/doctolib/doctolib/contents/packages/@doctolib/dmp/src/business/DmpApi.ts?ref=FHIR-2224-monolith-return-doc-status-update-results-from-dmp-api --jq '.content' 2>&1 | head -5 Tool results: → result (ok): (eval):1: no matches found: repos/doctolib/doctolib/contents/packages/@doctolib/dmp/src/business/DmpApi.ts?ref=FHIR-2224-monolith-return-doc-status-update-results-from-dmp-api Tools used: - Bash: gh api 'repos/doctolib/doctolib/contents/packages/%40doctolib/dmp/src/business/DmpApi.ts?ref=FHIR-2224-monolith-return-doc-status-update-results-from-dmp-api' --jq '.content' | base64 -d > /tmp/DmpApi …(truncated, 27 more chars) Tool results: → result (ok): 1049 /tmp/DmpApi.ts Tools used: - Bash: gh api 'repos/doctolib/doctolib/contents/packages/%40doctolib/dmp/src/types/DmpApi.ts?ref=FHIR-2224-monolith-return-doc-status-update-results-from-dmp-api' --jq '.content' | base64 -d > /tmp/DmpApi-ty …(truncated, 308 more chars) Tool results: → result (ok): 195 /tmp/DmpApi-types.ts 274 /tmp/DmpNhrAdapter.ts 469 total Tools used: - Read: /tmp/DmpApi-types.ts (170–195) Tool results: → result (ok): 170 171 export interface DmpApiErrorInfo { 172 apiErrorCode?: string 173 apiErrorType?: string 174 apiErrorContext?: string 175 apiErrorDescription?: string 176 apiErrorExtendedInformations?: string 177 apiErrorTypeDescription?: string 178 status: string 179 } 180 181 export interface SearchResult { 182 documents: DmpDocument[] 183 error?: DmpApiErrorInfo 184 } 185 186 export type DocumentStatusUpdateResult = { success: true } | { success: false; error: DmpApiErrorInfo } 187 …(truncated, 245 more chars) Tools used: - Read: /tmp/DmpApi.ts (340–415) Tool results: → result (ok): 340 341 const { sessionId } = cachedSession 342 const response = await getDocumentsContent({ sessionId, ...props }) 343 return response.Documents.map(this.toDocumentContent) 344 } 345 346 async updateDocumentStatus(props: DocumentStatusUpdateProps): Promise { 347 const { ins, documentUniqueId: uniqueId, uuid, getTransactionFrames } = props 348 if (!this.props.frDmpWorkflow) throw new Error('frDmpWorkflow is required to update document status' …(truncated, 2483 more chars) Tools used: - Read: /tmp/DmpNhrAdapter.ts (200–275) Tool results: → result (ok): 200 ins: toEnvIns(patient.nationalHealthIdentifier), 201 uniqueIds: params.ids, 202 getCdaContent: true, 203 getAuthors: true, 204 }) 205 return result.map((doc) => { 206 const typeCode = doc.cdaContent?.code?._code 207 const facet = findFacetForCDAType(typeCode, this.documentCategorizationFacets) 208 return toNhrDocumentContentResult(doc, facet) 209 }) 210 } 211 212 async updateDocumentMetadata(props: NhrDocumentMetadataUpdateProps): Pr …(truncated, 2141 more chars) Now I have enough context. Let me also check whether DmpApiError is the existing helper used elsewhere and also check the SearchResult error pattern for consistency. Tools used: - Bash: grep -rn "class DmpApiError" /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/packages/@doctolib/dmp/src/ 2>&1 | head -5 Tool results: → result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/packages/@doctolib/dmp/src/api/DmpApiError.ts:3:export class DmpApiError extends Error { Tools used: - Read: /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/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) Let me also see how buildUpdateResult works and verify if there's potential simplification on the construction of the response with all undefined fields. Tools used: - Bash: grep -n "i_apiErrorCode\|s_apiErrorContext\|UpdateDocumentStatusResponse\|s_status" /tmp/DmpApi.ts | head -20 Tool results: → result (ok): 289: if (status.s_status !== STATUS_OK) { 290: throw new Error(Error getting patient DMP Status, status: ${status.s_status}) 385: if (response.s_status !== STATUS_OK) { 389: apiErrorCode: response.i_apiErrorCode, 391: apiErrorContext: response.s_apiErrorContext, 395: status: response.s_status ?? 'ERROR', 426: if (authStatusResponse.s_status !== STATUS_OK) { 427: throw new Error(Error setting DMP Status, status: ${authStatusResponse …(truncated, 783 more chars) Tools used: - Read: /tmp/DmpApi.ts (575–600) Tool results: → result (ok): 575 submissionSetId: doc.s_submissionSetId, 576 uuid: doc.s_uuid, 577 previousUuid: doc.s_previousUuid, 578 nextUuid: doc.s_nextUuid, 579 documentStatus: doc.i_document_Status, 580 documentVisibility: doc.i_document_Visibility, 581 documentFormat: doc.i_document_Format, 582 authors: this.toDmpAuthors(doc.Authors ?? []), 583 eventCodes: this.toDmpEventCodes(doc.EventCodes), 584 handle: doc.i_handle, 585 referenceIds: this.toDmpRefe …(truncated, 621 more chars) I now have all I need. The search() method uses identical code to map response fields to DmpApiErrorInfo. Let me also verify whether the test file has any redundancy. Tools used: - Bash: gh api 'repos/doctolib/doctolib/contents/packages/%40doctolib/dmp/src/business/DmpApi.test.ts?ref=FHIR-2224-monolith-return-doc-status-update-results-from-dmp-api' --jq '.content' | base64 -d > /tmp/D …(truncated, 56 more chars) Tool results: → result (ok): }) expect(updateDocumentStatusCommandMocked).toHaveBeenCalledWith({ sessionId: 'session-abc', ins: '123456789012345678901', uniqueId: 'unique-doc-123', uuid: 'uuid-doc-456', healthCareSettings: 'SA07', getTransactionFrames: undefined, }) }) it('should return success: true on success', async () => { getCachedDmpSessionMocked.mockResolvedValue({ sessionId: 'session-abc', healthcareSettings: 'SA07' }) updateDocumentSt …(truncated, 2917 more chars) I have enough context to provide a thorough analysis. Let me also peek at the buildUpdateResult helper and look at the test setup for repeated calls. Tools used: - Read: /tmp/DmpNhrAdapter.ts (258–275) Tool results: → result (ok): 258 private buildUpdateResult(payloadResults: NhrDocumentMetadataUpdatePayloadResult[]): NhrDocumentMetadataUpdateResult { 259 const hasError = payloadResults.some((r) => r.status === 'error') 260 const hasSuccess = payloadResults.some((r) => r.status === 'success') 261 262 let status: NhrResultStatus 263 264 if (hasError && hasSuccess) { 265 status = 'partial success' 266 } else if (hasError) { 267 status = 'error' 268 } else { 269 status = 'success …(truncated, 94 more chars) I now have a comprehensive understanding. Here are my simplification suggestions for the PR. --- Code Simplification Review — PR #241133 (FHIR-2224) Analysis based on the latest commit on branch FHIR-2224-monolith-return-doc-status-update-results-from-dmp-api (tip: 52de63a3ad). High-impact suggestions 1. Extract a helper for mapping iCanopée response fields to DmpApiErrorInfo (DRY) File: packages/@doctolib/dmp/src/business/DmpApi.ts lines 386-396 (in updateDocumentStatus) and lines 587-598 (in the existing search mapping). Both blocks are identical seven-field mappings from s_/i_ response fields onto a DmpApiErrorInfo. The new code duplicates the mapping that already exists in search(). Pull this into a small helper (e.g. toDmpApiErrorInfo(response)) co-located with the response shape and reuse in both call sites. This: - Removes ~10 lines of duplication. - Centralizes the ?? 'ERROR' fallback and field-name normalization in one place, so future iCanopée additions (or renames) only change one spot. Suggested signature: function toDmpApiErrorInfo(response: { s_status?: string; i_apiErrorCode?: string; ... }): DmpApiErrorInfo --- 2. Two near-identical catch blocks — collapse into a single helper File: packages/@doctolib/dmp/src/business/DmpApi.ts lines 363-366 (dmpService path) and lines 401-404 (legacy path). const message = e instanceof Error ? e.message : 'Unknown error' return { success: false, error: { status: 'ERROR', apiErrorDescription: message } } Both paths use the exact same two lines. Extract a tiny helper such as toUnknownErrorResult(e: unknown): DocumentStatusUpdateResult (or inline as a local arrow inside the method). Bonus: the DmpApiError class already encapsulates the same fallback string 'DMP API error'; consider whether 'Unknown error' should reuse that wording for consistency with the rest of the package. --- 3. BusinessError is no longer special-cased — verify intent / remove import if unused File: packages/@doctolib/dmp/src/business/DmpApi.ts line 363-366. The previous code distinguished BusinessError from arbitrary errors. The new code catches any Error and maps e.message blindly. If the goal of the PR was to keep that distinction (the PR description mentions "Non-BusinessError still propagates"), the current implementation diverges from the description — it now swallows every error, including programmer errors, network errors, etc. Either: - Restore the instanceof BusinessError guard and throw e re-throw for non-business errors, or - Confirm "fully error-as-data" is the intended new behavior and remove the now-unused BusinessError import if it's no longer referenced (check import { BusinessError } near the top of the file). This is a behavioral correctness comment more than a simplification, but it's load-bearing for the PR's contract. --- 4. DmpNhrAdapter.updateDocumentMetadata — flatten nested branches File: packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts lines 220-253. The inner payloads.map callback can be tightened by extracting per-payload handling into a small named function. Right now the closure has three levels of indentation, mixes the "status" branch and the "skipped/visibility" branch, and re-builds object literals with the same keys (id, uuid, propertyName) four times. Two options: Option A — extract a helper: async function processPayload(payload: ..., ins: string): Promise { const { id, uuid, propertyName } = payload if (propertyName !== 'status') { return { id, uuid, propertyName, status: 'skipped', message: Unsupported propertyName: ${propertyName} } } const result = await this.dmpApi.updateDocumentStatus({ ins, documentUniqueId: id, uuid: uuid ?? '' }) if (!result.success) { return { id, uuid, propertyName, status: 'error', message: new DmpApiError(result.error).message } } return { id, uuid, propertyName, status: 'success' } } Then payloads.map((p) => processPayload(p, ins)). Option B — early return on the non-status branch first, which removes one level of nesting in the existing inline callback. Either way reduces visual noise without changing behavior. --- 5. buildUpdateResult — replace if/else chain with a direct ternary or lookup File: packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts lines 258-273. let status: NhrResultStatus if (hasError && hasSuccess) status = 'partial success' else if (hasError) status = 'error' else status = 'success' Pre-existing code (not changed in this PR) but flagging it as part of the surrounding context: this is fine as-is per the project rule "avoid nested ternaries — prefer if/else chains for multiple conditions." Leave as is. (No action.) --- 6. Test setup duplication — extract a helper for the repeated updateDocumentStatus invocation File: packages/@doctolib/dmp/src/business/DmpApi.test.ts lines 1612-1685 (and similar block around lines 2047-2098 in the dmpService describe). Each it block repeats: const result = await dmpApi.updateDocumentStatus({ ins: '123456789012345678901', documentUniqueId: 'unique-doc-123', uuid: 'uuid-doc-456', }) That's the same literal DocumentStatusUpdateProps four times in each describe block. A const updateProps = { ... } constant at the top of the describe (or a callUpdate(api) helper) would shave a dozen lines and make each test focus on what's unique (mock setup + assertion). --- 7. Test file — expect.objectContaining is unnecessary when comparing to a fully-known shape File: packages/@doctolib/dmp/src/business/DmpApi.test.ts lines ~1638, 1660, 1683, etc. — and analogous lines in DmpNhrAdapter.test.ts. expect(result).toEqual({ success: false, error: expect.objectContaining({ status: 'ERROR', apiErrorDescription: 'No DMP session found' }), }) Because the implementation builds the error literal with exactly { status: 'ERROR', apiErrorDescription: '...' } (no other keys), expect.objectContaining is redundant — a plain toEqual of the full object would assert the same thing more strictly and read more naturally. Use objectContaining only where future-proofing against extra fields matters; here you're testing the exact branch that produces the literal. Keep objectContaining for the iCanopée non-OK case (where the response shape may legitimately carry other fields), drop it for the simple 'ERROR' paths. --- 8. Type design — DocumentStatusUpdateResult discriminated union vs. SearchResult shape File: packages/@doctolib/dmp/src/types/DmpApi.ts line 186. export type DocumentStatusUpdateResult = { success: true } | { success: false; error: DmpApiErrorInfo } The PR description claims this "mirrors the existing SearchResult pattern", but SearchResult is { documents: ...; error?: DmpApiErrorInfo } — an optional-error object, not a discriminated union. Two consistency choices: - Match SearchResult exactly (what the PR description says): interface DocumentStatusUpdateResult { error?: DmpApiErrorInfo }. Callers check result.error (already what DmpNhrAdapter did in earlier commits). - Migrate SearchResult to a discriminated union too, so the codebase is consistent the other way. The discriminated union is arguably better type design (forces callers to check before accessing error). But mixing two patterns within the same file/package for the same concept (DMP API result-or-error) is the worst of both worlds. Pick one and align both. Given that PR #240721 established SearchResult first, the lighter-touch fix is to keep DocumentStatusUpdateResult = { error?: DmpApiErrorInfo } and have the caller check if (result.error) — which is what the adapter actually wants and removes the need for the succ …(truncated, 2317 more chars)