Project: treatment-plan · Branch: main · Mode: · Model: claude-sonnet-4-6 Stats: 1 user messages, 14 tool calls, tools used: Bash, Read. Conversation Turn 1 — User Review the updated PR https://github.com/doctolib/doctolib/pull/240556 (branch fhir-2216_fix-nhr-date-columns-and-document-type) — second pass. Previous review found two comment-smell issues which have been fixed. The main code change since first review: parseHl7DateToIso in DmpNhrAdapter.ts now: 1. Appends Z to 14-char HL7 datetime ISO output (UTC timezone fix) 2. Adds /^\d{14}$/ and /^\d{8}$/ regex guards Files changed (all in packages/@doctolib/national-health-records/src/): - business/adapters/DmpNhrAdapter.ts — UTC fix + regex guards - business/adapters/DmpNhrAdapter.test.ts — updated assertions + 2 new tests - variants/dmp/mappers/dmpDocumentHelpers.ts — comment fixes (already reviewed) - variants/dmp/mappers/dmpDocumentHelpers.test.ts — 1 new test (already reviewed) - mocks/nhrMockedDocuments.ts — mock realignment (already reviewed) Focus on: UpFront architecture compliance, naming conventions, separation of concerns, and any remaining code smell. Report only actionable items not already addressed. Turn 1 — Assistant I'll start by fetching the PR diff and examining the relevant files. Tools used: - Bash: cd /Users/USER/workspace/doctolib && gh pr view 240556 --json files,headRefName,baseRefName 2>/dev/null | head -100 Tools used: - Bash: cd /Users/USER/workspace/doctolib && gh pr diff 240556 2>/dev/null | head -400 Tool results: → result (ok): (Bash completed with no output) Tool results: → result (ok): (Bash completed with no output) The gh CLI doesn't have access to the doctolib/doctolib repo from this workspace. Let me access the files directly via the worktree. Tools used: - Bash: ls /Users/USER/workspace/doctolib/worktrees/doctolib/ 2>/dev/null Tool results: → result (ok): fhir-2089 FHIR-2091-nhr-document-list-pagination FHIR-2166-fix-nhr-mock-status-visibility fhir-2169 fhir-2181 fhir-2189_class-code-label-map fhir-2190_improve-filter-panel-ui fhir-2209_fix-patient-ins-env-transformation fhir-2216_fix-nhr-date-columns-and-document-type Tools used: - Bash: find /Users/USER/workspace/doctolib -maxdepth 4 -name "DmpNhrAdapter.ts" 2>/dev/null Tool results: → result (ok): (Bash completed with no output) Tools used: - Bash: find /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2216_fix-nhr-date-columns-and-document-type -name "DmpNhrAdapter*" 2>/dev/null Tool results: → result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2216_fix-nhr-date-columns-and-document-type/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2216_fix-nhr-date-columns-and-document-type/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.test.ts Tools used: - Read: /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2216_fix-nhr-date-columns-and-document-type/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, 7500 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2216_fix-nhr-date-columns-and-document-type/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, 16764 more chars) Now let me look at the other changed files and compare with main branch to get the full diff picture. Tools used: - Bash: cd /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2216_fix-nhr-date-columns-and-document-type && git diff master -- packages/@doctolib/national-health-records/src/business/adapters/DmpNhr …(truncated, 22 more chars) Tools used: - Bash: cd /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2216_fix-nhr-date-columns-and-document-type && git diff master -- packages/@doctolib/national-health-records/src/business/adapters/DmpNhr …(truncated, 27 more chars) 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 231be9f5635b..11c30ab3f360 100644 --- a/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts +++ b/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts @@ -19,6 +19,8 @@ import type { NhrDocumentContentResult, NhrDocumentFile, NhrGetDocumentsConten …(truncated, 3396 more chars) 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 e2e32a1176c0..9a3dbef74bdf 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 @@ -49,6 +49,7 @@ const dmpSearchResult: DmpSearchResult = { classCode: '11488 …(truncated, 6066 more chars) Tools used: - Bash: find /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2216_fix-nhr-date-columns-and-document-type/packages/@doctolib/national-health-records/src -type f | sort 2>/dev/null Tool results: → result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2216_fix-nhr-date-columns-and-document-type/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.test.ts /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2216_fix-nhr-date-columns-and-document-type/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2216_fix-nhr-date-columns-and-document-type/packages/@doctolib/national-health …(truncated, 6236 more chars) Tools used: - Bash: cd /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2216_fix-nhr-date-columns-and-document-type && git diff master -- packages/@doctolib/national-health-records/src/variants/dmp/mappers/dmp …(truncated, 200 more chars) Tool results: → result (ok): diff --git a/packages/@doctolib/national-health-records/src/mocks/nhrMockedDocuments.ts b/packages/@doctolib/national-health-records/src/mocks/nhrMockedDocuments.ts index 909afcaa3827..42f8763a3dc1 100644 --- a/packages/@doctolib/national-health-records/src/mocks/nhrMockedDocuments.ts +++ b/packages/@doctolib/national-health-records/src/mocks/nhrMockedDocuments.ts @@ -523,37 +523,37 @@ export const nhrMockedDocuments: NhrDocument[] = [ ], }, }, - // ── Batch 9: Pathology reports …(truncated, 26615 more chars) Now let me check the package structure and any remaining files in scope. Tools used: - Bash: ls /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2216_fix-nhr-date-columns-and-document-type/packages/@doctolib/national-health-records/ 2>/dev/null Tools used: - Read: /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2216_fix-nhr-date-columns-and-document-type/packages/@doctolib/national-health-records/src/variants/dmp/mappers/dmpDocumentHelpers.ts Tool results: → result (ok): 1 import type { NhrDocument } from '@doctolib/tpf-national-health-records' 2 import type { NhrVisibilityEntry } from '@doctolib/tpf-national-health-records-ui' 3 import { DOCUMENT_STATUS, DOCUMENT_VISIBILITY } from '@doctolib/dmp' 4 5 const CLASS_CODE_SYSTEM = 'dmp:classCode' 6 const TYPE_CODE_SYSTEM = 'dmp:typeCode' 7 const PATIENT_CODE = '90' // EX_3.1-1040: class code for patient-authored documents 8 9 // classification.codes may contain both a classCode (JDV_J06) and a typeCode (JDV_J07/LO …(truncated, 2792 more chars) Tool results: → result (ok): babel.config.cjs catalog-info.yaml CODEOWNERS.local i18n.json jest.config.cjs package.json src tsconfig.json I now have everything I need. Let me do a final check on the test to ensure there is no redundancy issue with the length check and the regex. Tools used: - Bash: node -e " … Tool results: → result (ok): 14-char regex: true length: 14 8-char regex: true length: 8 6-char bad: false false alpha8 bad: false false badAlpha8 length: 8 I have everything needed for a complete review. Here is the second-pass assessment. --- Architecture Review — PR #240556 (second pass) Critical Issues (must fix) None. The separation-of-concerns boundaries are respected: DmpNhrAdapter.ts remains in src/business/adapters/, contains no React imports, and performs only pure data transformation. dmpDocumentHelpers.ts is correctly placed in the variant's mappers/ folder. No data-layer logic has leaked into business, or vice versa. --- Warnings (should fix) 1. Redundant .length pre-checks before regex guards in parseHl7DateToIso /^\d{14}$/ already implies .length === 14, and /^\d{8}$/ implies .length === 8. The explicit length comparisons on lines 94 and 101 add noise without adding safety. // current if (hl7Date.length === 14 && /^\d{14}$/.test(hl7Date)) { if (hl7Date.length === 8 && /^\d{8}$/.test(hl7Date)) { // correct — regex is the complete guard if (/^\d{14}$/.test(hl7Date)) { if (/^\d{8}$/.test(hl7Date)) { This is not a performance concern — the regex anchors make it equivalent. The issue is readability: the length check implies the regex is not trusted, which is confusing. 2. Missing test for an 8-char non-numeric 14-char-looking string via parseHl7DateToIso The existing "returns undefined for a non-numeric 8-char string" test uses 'Lundi-15' (which is length 8 and contains a hyphen — the regex catches it). There is no test for a 14-char non-numeric input such as '2023090T083015'. Given the guard was specifically added to protect the 14-char path, the symmetry with the 8-char negative test is missing. it('returns undefined for a non-numeric 14-char string', async () => { const result = await new DmpNhrAdapter( createMockDmpApi({ documents: [{ ...baseDoc, creationDate: '2023-09T08301' }] }) ).searchDocuments(searchProps) expect(result.documents[0]?.creationDate).toBeUndefined() }) File: /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2216_fix-nhr-date-columns-and-document-type/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.test.ts, inside describe('HL7 date conversion in toNhrDocument'). --- Suggestions (consider improving) 3. parseHl7DateToIso is a pure, portable utility — consider moving it to src/helpers/ The function has no dependency on DmpNhrAdapter internals and no coupling to the DMP API type. It operates solely on a raw string. It is currently file-private (not exported), which is fine for now, but if date handling expands to other adapters (NHR variants, future registries), it will be duplicated. Noting for when a second caller appears — do not move it preemptively. 4. updateDocumentMetadata stub on DmpNhrAdapter (line 205–207) The method throws 'Method not implemented.' unconditionally. This is acceptable as a placeholder per the comment, but the method should ideally be annotated with the ticket reference (FHIR-2193 was mentioned in the removed @ts-expect-error) so the stub is traceable: // To be implemented in FHIR-2193 updateDocumentMetadata(_props: NhrDocumentMetadataUpdateProps): Promise { throw new Error('Method not implemented.') } The comment currently says "a separate ticket" without an ID. The removed suppression comment referenced FHIR-2193 — that ID should be preserved here. 5. Test fixture mutation pattern — { ...baseDoc, creationDate: '...' } repeated across 9 cases The spread is correct and each case is independent, which is good. The pattern is idiomatic but the repeated createMockDmpApi({ documents: [{ ...baseDoc, X }] }).searchDocuments(searchProps) call chain inside every it body is verbose. A small local helper would improve readability at no architecture cost: const resolveDate = async (overrides: Partial) => new DmpNhrAdapter(createMockDmpApi({ documents: [{ ...baseDoc, ...overrides }] })) .searchDocuments(searchProps) .then(r => r.documents[0]) This is a style suggestion, not a rule violation. --- Summary The two substantive items from the previous review (comment smell) have been correctly resolved. The new logic (parseHl7DateToIso with UTC Z suffix and regex guards) is architecturally sound and correctly placed. The only actionable items are: dropping the redundant length checks (warning), adding the symmetric 14-char non-numeric negative test (warning), and restoring the FHIR-2193 ticket reference on the stub method (suggestion).