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 Analyze the type design of new types introduced in PR #241133 (FHIR-2224) in the doctolib/doctolib monorepo. Worktree path: /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/ New type introduced: // packages/@doctolib/dmp/src/types/DmpApi.ts lines ~186–188 export interface DocumentStatusUpdateResult { error?: DmpApiErrorInfo } Existing reference type (for comparison): export interface SearchResult { documents: DmpDocument[] error?: DmpApiErrorInfo } Type design questions to evaluate: 1. Structural redundancy: DocumentStatusUpdateResult { error?: DmpApiErrorInfo } is isomorphic to { error?: DmpApiErrorInfo }. The interface adds no invariants beyond a type alias. Would type DocumentStatusUpdateResult = { error?: DmpApiErrorInfo } or reusing Omit be better? Does the named interface add meaningful expressiveness? 2. Empty success representation: {} represents success, but TypeScript will accept any object as DocumentStatusUpdateResult since error is optional. Should success be more explicitly typed (e.g., discriminated union { success: true } | { success: false; error: DmpApiErrorInfo })? 3. DmpApiErrorInfo in the dmpService path: The returned error { status: 'ERROR', apiErrorDescription: e.message } only populates 2 of the 7 DmpApiErrorInfo fields — the other 5 are undefined. The type doesn't distinguish between "not available" and "not applicable". Is this a type invariant violation? 4. Interface vs type alias: DocumentStatusUpdateResult uses interface while some other simple types in the same file use type. Is there a reason for this choice? 5. Export surface: DocumentStatusUpdateResult is exported from both packages/@doctolib/dmp/src/business/DmpApi.ts (re-export) AND packages/@doctolib/dmp/src/index.ts. Is double-export intentional or redundant? Read the actual files at the worktree path and provide analysis with ratings (1-5) for: encapsulation, invariant expression, usefulness, enforcement. Report in Conventional Comments format with file:line references. Turn 1 — Assistant I'll read the relevant files to perform a thorough analysis before drawing conclusions. Tools used: - Bash: find /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/packages/@doctolib/dmp/src -type f | sort Tool results: → result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/packages/@doctolib/dmp/src/api/auth-types.ts /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/packages/@doctolib/dmp/src/api/connectToDmp.test.ts /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/packages/@doctolib/dmp/src/api/connectToDmp.ts /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/packages/@doctolib/dmp/src/api/DmpApiError.test.ts /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/packa …(truncated, 6863 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) Tools used: - Read: /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/packages/@doctolib/dmp/src/business/DmpApi.ts Tools used: - Read: /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/packages/@doctolib/dmp/src/index.ts 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 { getLoincCode } from 'monolith_DEPRECATED/models/document/document' 5 import { sendMonitoringEvent } from 'monolith_DEPRECATED/services/monitoring' 6 import { isMinor } from 'monolith_DEPRECATED/models/patient/patient' 7 import { isMedicalSynthesis } from 'monolith_DEPRECATED/components/do …(truncated, 38564 more chars) Tool results: → result (ok): 1 export { 2 type DmpDocumentItem, 3 type DmpDocumentToAdd, 4 type DmpDocumentToDelete, 5 type FrDmpDocument, 6 ItemType, 7 type UploadedDmpDocumentItem, 8 } from './business/FrDmpConfig' 9 export { getErrorMessage, getWarningMessage } from './business/DmpI18nMessage' 10 export type { 11 DmpApi, 12 DmpAuthor, 13 DmpDocument, 14 DmpDocumentContent, 15 DmpSetUpProps, 16 ShareProps, 17 UnshareProps, 18 SearchProps, 19 SearchResult, 20 DocumentStatusUpdateProps, 21 …(truncated, 5772 more chars) Now let me read the FrDmpTypes to understand UpdateDocumentStatusByUniqueIdResponse, and then look at the consumers of DocumentStatusUpdateResult to understand how the type is actually used. Tools used: - Read: /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/packages/@doctolib/dmp/src/types/FrDmpTypes.ts 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 healthcareSettings: string 9 createdAt: Date 10 } 11 12 // DMP Requests 13 export interface DmpRequest { 14 s_commandName: string 15 } 16 …(truncated, 24421 more chars) Tools used: - Bash: grep -rn "DocumentStatusUpdateResult\|updateDocumentStatus" /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/ --include=".ts" --include=".tsx" | grep -v node_modules | grep -v ".d.ts …(truncated, 8 more chars) Tool results: → result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/packages/@doctolib/dmp-validation/src/components/DmpValidation/ValidationPage/commandHandlers.ts:43: hl_updateDocumentStatusByUniqueId: (sessionId, params) => /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/packages/@doctolib/dmp-validation/src/components/DmpValidation/ValidationPage/commandHandlers.ts:44: updateDocumentStatus({ sessionId, ...params } as Parameters[0]), /Users/USER/workspace/ …(truncated, 8985 more chars) Now let me read the consuming adapter and its test to understand how DocumentStatusUpdateResult is actually used in practice. Tools used: - Read: /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts (200–260) Tool results: → result (ok): 200 const typeCode = doc.cdaContent?.code?._code 201 const facet = findFacetForCDAType(typeCode, this.documentCategorizationFacets) 202 return toNhrDocumentContentResult(doc, facet) 203 }) 204 } 205 206 async updateDocumentMetadata(props: NhrDocumentMetadataUpdateProps): Promise { 207 if (!props.patient?.nationalHealthIdentifier) { 208 throw new Error('patient_national_health_identifier_required') 209 } 210 211 const …(truncated, 1717 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/packages/@doctolib/dmp/src/business/DmpApi.test.ts (1585–1685) Tool results: → result (ok): 1585 await expect(dmpApi.updatePatientConsent('add')).rejects.toThrow('Error setting DMP Status, status: ERROR') 1586 }) 1587 }) 1588 1589 describe('updateDocumentStatus ', () => { 1590 it('should call updateDocumentStatusCommand with session and props', async () => { 1591 getCachedDmpSessionMocked.mockResolvedValue({ sessionId: 'session-abc', healthcareSettings: 'SA07' }) 1592 updateDocumentStatusCommandMocked.mockResolvedValue({ s_status: STATUS_OK }) 1593 1594 …(truncated, 3402 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/packages/@doctolib/dmp/src/business/DmpApi.test.ts (2005–2085) Tool results: → result (ok): 2005 2006 expect(result).toEqual([]) 2007 }) 2008 }) 2009 2010 describe('updateDocumentStatus (UpdateDocumentStatus)', () => { 2011 it('should call dmpService.call with UpdateDocumentStatus action and correct payload', async () => { 2012 mockDmpService.call.mockResolvedValue({}) 2013 2014 const dmpApi = createDmpApiWithService() 2015 await dmpApi.updateDocumentStatus({ 2016 ins: '123456789012345678901', 2017 documentUniqu …(truncated, 2837 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.test.ts (535–655) Tool results: → result (ok): 535 536 await expect(adapter.updateDocumentMetadata({ ...updateProps, patient: undefined })).rejects.toThrow( 537 'patient_national_health_identifier_required' 538 ) 539 }) 540 541 it('should call dmpApi.updateDocumentStatus with correct params for status payload', async () => { 542 const mockDmpApi = createMockDmpApi() 543 const adapter = new DmpNhrAdapter(mockDmpApi, undefined) 544 545 await adapter.updateDocumentMetadata(updateProps) 546 547 …(truncated, 4740 more chars) Now let me look at the DmpApiError class to understand how it consumes DmpApiErrorInfo, and also check the DmpApi interface and the DmpApiErrorInfo field layout more carefully. 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) Tools used: - Bash: grep -n "DocumentStatusUpdateResult\|DocumentVisibilityUpdateResult\|type.Result\|interface.Result" /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/packages/@doctolib/dmp/src/types/ …(truncated, 9 more chars) Tool results: → result (ok): 112: updateDocumentStatus: (props: DocumentStatusUpdateProps) => Promise 181:export interface SearchResult { 186:export interface DocumentStatusUpdateResult { Tools used: - Bash: grep -n "interface.Result\|type.Result" /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/packages/@doctolib/dmp/src/types/FrDmpTypes.ts Tool results: → result (ok): 2:import type { DmpResult } from '@doctolib/tpf-french-teleservices' 192:export interface DmpResponse extends DmpResult { 447:export interface DocumentUploadResult { Tools used: - Bash: cd /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224 && git log --oneline --diff-filter=A -- "packages/@doctolib/dmp/src/types/DmpApi.ts" "packages/@doctolib/dmp/src/business/DmpApi.ts" …(truncated, 23 more chars) Tool results: → result (ERROR): Permission to use Bash has been denied. IMPORTANT: You may attempt to accomplish this action using other tools that might naturally be used to accomplish this goal, e.g. using head instead of cat. But you should not attempt to work around this denial in malicious ways, e.g. do not use your ability to run tests to execute non-test actions. You should only try to work around this restriction in reasonable ways that do not attempt to bypass the intent behind this denial. If you believe this cap …(truncated, 178 more chars) I have now read all the relevant files. Here is the full analysis. --- Type: DocumentStatusUpdateResult File: /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2224/packages/@doctolib/dmp/src/types/DmpApi.ts lines 186–188 --- Invariants Identified - Implicit success invariant: {} (no error field) means the operation succeeded. This is a convention, not an encoded invariant. TypeScript cannot distinguish {} from { error: undefined }, and any object is structurally assignable to this type. - Implicit error invariant: When error is present, the status field within DmpApiErrorInfo is required (status: string), but the six other fields are all optional. The type therefore permits error objects where only { status: 'ERROR' } is populated — which is exactly what the BusinessError catch branch at DmpApi.ts:363 produces. - No completion/transition invariant: The type cannot express that, once returned, this is a terminal result (not an intermediate state). --- Ratings Encapsulation: 2/5 The type is a plain interface with a single optional property. It exposes no behaviour, and the convention that {} means success is entirely invisible from the definition. Any external code can construct a DocumentStatusUpdateResult literal with or without error, including one with error: undefined, and all three are structurally identical to TypeScript. The "empty = success" contract lives in prose and test comments, not in the type itself. Invariant Expression: 2/5 The critical business distinction — "did the DMP operation succeed?" — is expressed only as the absence of a property. There is no type-level encoding of success | error. Callers must check result.error (as DmpNhrAdapter.ts:228 does), and that check is easy to forget. In contrast, SearchResult carries both documents and error?, which at minimum forces callers to handle the documents field, but DocumentStatusUpdateResult has nothing on the happy path to make the consumer think twice. Invariant Usefulness: 3/5 The type does provide real value in one narrow sense: it allows the dmpService path to return a structured BusinessError instead of throwing, matching the behaviour of the legacy path, and it allows DmpNhrAdapter to branch on result.error without catching exceptions. This dual-mode (thrown vs. returned error) design is the driver for the type's existence. However the usefulness is partially undermined by the DmpApiErrorInfo sparseness issue described below. Invariant Enforcement: 2/5 There is zero compile-time or runtime enforcement that the returned object is either a proper success or a proper error. The BusinessError path at DmpApi.ts:363 constructs { error: { status: 'ERROR', apiErrorDescription: e.message } }, leaving five of the seven DmpApiErrorInfo fields undefined. TypeScript accepts this because all five are ? fields. There is no factory, no validation, and no assertNever-style exhaustion. A caller could write return {} where it should write return { error: ... } and the compiler would not object. --- Strengths - The named interface makes intent searchable and mockable. jest.fn().mockResolvedValue({}) in every test file (DmpApi.test.ts:1624, DmpNhrAdapter.test.ts:43) is unambiguous precisely because a named return type exists. A bare Promise<{ error?: DmpApiErrorInfo }> would still compile but would make mock factories harder to read. - The type is consistent with the pattern already established by SearchResult (lines 181–184) in the same file: both carry an optional error?: DmpApiErrorInfo, and keeping the error representation uniform makes DmpApiError (which accepts DmpApiErrorInfo) reusable across both call sites without adapter logic. - DocumentStatusUpdateProps (lines 192–198) is paired with DocumentStatusUpdateResult on the DmpApi interface (line 112), giving a clear request/response pair that makes the API contract readable at a glance. --- Concerns suggestion (structural) DmpApi.ts:186–188 — The interface adds no invariants beyond { error?: DmpApiErrorInfo }. Its single field is identical in structure to Omit. This is not objectionable on its own — named types aid discoverability — but it does mean the interface declaration itself teaches the reader nothing about when the type is valid. The expressiveness is entirely borrowed from the name, not from the structure. issue (invariant violation) DmpApi.ts:363 — The BusinessError catch branch constructs { error: { status: 'ERROR', apiErrorDescription: e.message } }, populating 2 of the 7 DmpApiErrorInfo fields. DmpApiError (DmpApiError.ts:18–32) handles this gracefully via ?? chains, but the type system does not distinguish "field absent because the teleservice did not return it" from "field absent because this is a BusinessError and the rich metadata was never available." A reader looking at DmpApiErrorInfo has no idea some error paths will always have five undefined fields. This is a documentation debt masquerading as a type design. issue (success representation) DmpApi.ts:186–188 — {} is a valid DocumentStatusUpdateResult, a valid SearchResult, and a valid assignment target for any object type in TypeScript's structural system. Tests at DmpApi.test.ts:1624 and DmpApi.test.ts:2044 assert expect(result).toEqual({}) for the success case. This confirms the {} convention is intentional but also that it is entirely invisible from the type. A consumer who forgets to check result.error will silently treat all outcomes as success. nitpick (interface vs type alias) DmpApi.ts:186 — The same file uses type for PatientConsent, PatientConsentUpdate, DmpReferenceId, ShareProps, UnshareProps, SearchProps, DocumentStatusUpdateProps, GetDocumentsContentProps, and DmpSetUpProps, while using interface for DmpAddress, DmpTelecom, DmpInstitution, DmpAuthor, DmpDocument, DmpDocumentContent, DmpApi, DmpApiErrorInfo, SearchResult, and DocumentStatusUpdateResult. The split appears to track "data shapes that represent API objects" (interface) vs. "union types and function prop bags" (type). DocumentStatusUpdateResult fits the interface pattern, so the choice is consistent with local convention, though the distinction is not enforced anywhere. nitpick (export surface) The type is exported from three locations: its source (types/DmpApi.ts line 186), the business barrel (business/DmpApi.ts line 102–117), and the package index (index.ts line 21). The business barrel's export type { ... } from '../types/DmpApi' is a re-export pass-through, not a duplication of the definition. index.ts then re-exports from ./types/DmpApi directly (line 22), bypassing the business barrel. This means there are two distinct public import paths for the same type, which can lead to implicit coupling if the two paths are ever kept in sync manually. The business barrel re-export is the more concerning of the two: it exports DocumentStatusUpdateResult alongside implementation details (DmpApiImpl), which blurs the boundary between the type layer and the implementation layer. --- Recommended Improvements 1. Replace the implicit success convention with a discriminated union (moderate effort, high payoff) The clearest upgrade for invariant expression is: export type DocumentStatusUpdateResult = | { success: true } | { success: false; error: DmpApiErrorInfo } This makes illegal states unrepresentable: a caller cannot receive a result without knowing which branch they are in, if (result.success) is self-documenting, and the error field only exists when it is meaningful. The DmpNhrAdapter.ts:228 check if (result.error) becomes if (!result.success), which is equally readable but now type-narrowed. If the team prefers to stay wit …(truncated, 2015 more chars)