Project: doctolib-worktrees · Branch: HEAD · Mode: · Model: claude-sonnet-4-6 Stats: 1 user messages, 8 tool calls, tools used: Bash, Read. Conversation Turn 1 — User You are performing a code review for PR #239184 in doctolib/doctolib. Your job is to do a shallow scan for obvious bugs in the changed code. Worktree path: /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2181 Head SHA: 3affc72b3fed042d4589a040d638483a237d90c3 Changed files: - packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.test.ts - packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.ts - packages/@doctolib/national-health-records/src/mocks/filterMockedDocuments.test.ts - packages/@doctolib/national-health-records/src/mocks/filterMockedDocuments.ts - packages/@doctolib/national-health-records/src/mocks/generateNhrDocuments.ts - packages/@doctolib/national-health-records/src/mocks/nhrMockedDocuments.ts - packages/@doctolib/national-health-records/src/variants/dmp/mappers/mapFiltersToSearchDocumentsParams.test.ts - packages/@doctolib/national-health-records/src/variants/dmp/mappers/mapFiltersToSearchDocumentsParams.ts PR Summary: This PR fixes 4 bugs: 1. mapStatusFilter was always including validated in bitmask. Fixed to return undefined when no filters selected. 2. createNhrDateSelectionFilterConfig emits dateType as string[] but mapDateFilters compared as scalar. Added resolveDateType() helper. 3. useSearchDocuments called generateNhrDocuments() without passing appliedFilters in mock mode. Fixed. 4. Mock documents used system: 'LOINC' but getTypeCode() expects system: 'dmp:classCode'. Fixed. Steps: 1. Get the PR diff: gh pr diff 239184 --repo doctolib/doctolib 2. Read each changed file from the worktree to understand the full context of changes 3. Scan for obvious bugs in the changes only (not pre-existing issues) Focus on large bugs, avoid nitpicks. Ignore issues linters/compilers would catch. Return a list of specific bugs found with file and line references. If no bugs, say so. Turn 1 — Assistant I'll start by getting the PR diff to understand what changed. Tools used: - Bash: gh pr diff 239184 --repo doctolib/doctolib 2>&1 | head -500 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 c43d23517926e..c587d51fa1366 100644 --- a/packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.test.ts +++ b/packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.test.ts @@ -6,10 +6,12 @@ import type { Patient } from 'monolith_DEPRECATED/types/doctor/api/Patient' import type { Nh …(truncated, 24212 more chars) Tools used: - Bash: gh pr diff 239184 --repo doctolib/doctolib 2>&1 | tail -300 Tool results: → result (ok): { id: 'mock-014', @@ -175,7 +175,7 @@ export const nhrMockedDocuments: NhrDocument[] = [ format: 'application/pdf', serviceInformation: { startDate: '2024-10-07' }, authors: [{ id: 'doc-11', firstName: 'Hélène', lastName: 'Blanc', role: 'healthcare_professional' }], - classification: { codes: [{ system: 'LOINC', code: 'Consultation report' }] }, + classification: { codes: [{ system: 'dmp:classCode', code: 'Consultation report' }] }, }, { id: 'mock-015', @@ -1 …(truncated, 14946 more chars) Now let me read the full filterMockedDocuments.ts and mapFiltersToSearchDocumentsParams.ts files for complete context. Tools used: - Read: /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2181/packages/@doctolib/national-health-records/src/mocks/filterMockedDocuments.ts Tool results: → result (ok): 1 import type { NhrDocument } from '@doctolib/tpf-national-health-records' 2 import { DOCUMENT_STATUS, DOCUMENT_VISIBILITY } from '@doctolib/dmp' 3 4 import type { ServerFilters } from '../variants/types' 5 6 / 7 Applies ServerFilters to a list of mock NhrDocuments, simulating DMP server-side filtering. 8 Used so that the mock path in useSearchDocuments respects filter changes the same way the real API does. 9 */ 10 export function filterMockedDocuments(docs: NhrDocument[], filters: S …(truncated, 3173 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2181/packages/@doctolib/national-health-records/src/variants/dmp/mappers/mapFiltersToSearchDocumentsParams.ts Tool results: → result (ok): 1 import type { NhrDocumentSearchParams } from '@doctolib/tpf-national-health-records' 2 import { DOCUMENT_STATUS, DOCUMENT_VISIBILITY } from '@doctolib/dmp' 3 import type { ServerFilters } from '../../types' 4 import { toNhrPatientInformation } from '../../../business/mappers/toNhrPatientInformation' 5 6 // Maps UI-level ServerFilters to the NhrDocumentSearchParams format expected by the NHR API. 7 // Each ServerFilters field needs an explicit mapping — field names do not match NhrDocumentSear …(truncated, 3162 more chars) Now let me read the ServerFilters type and the nhrMockedDocuments.ts to verify the code change context: Tools used: - Read: /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2181/packages/@doctolib/national-health-records/src/variants/types.ts Tool results: → result (ok): 1 import type { ColumnConfig, FilterConfig } from '@doctolib/tpf-national-health-records-ui' 2 import type { Patient } from 'monolith_DEPRECATED/types/doctor/api/Patient' 3 import type { NhrDocument, NhrDocumentSearchParams } from '@doctolib/tpf-national-health-records' 4 5 interface SearchDocumentsResult { 6 data: NhrDocument[] 7 total: number 8 } 9 10 export type NhrDocumentListVariant = { 11 columns: ColumnConfig[] 12 filters: FilterConfig[] 13 searchDocuments: (appliedFilters?: R …(truncated, 791 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2181/packages/@doctolib/national-health-records/src/mocks/nhrMockedDocuments.ts (1–51) Tool results: → result (ok): 1 import type { NhrDocument } from '@doctolib/tpf-national-health-records' 2 3 export const nhrMockedDocuments: NhrDocument[] = [ 4 // ── Batch 1: Laboratory reports (6 docs) ────────────────────────────────── 5 { 6 id: 'mock-001', 7 title: 'SOPK hormone panel', 8 creationDate: '2025-01-13', 9 submissionDate: '2025-01-18', 10 status: '1', 11 visibility: '1', 12 format: 'application/pdf', 13 serviceInformation: { startDate: '2025-01-10' }, 14 authors: [{ i …(truncated, 1535 more chars) Now I have a clear picture. Let me analyze the key logic carefully: Tools used: - Read: /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2181/packages/@doctolib/national-health-records/src/mocks/filterMockedDocuments.test.ts (100–150) Tool results: → result (ok): 100 }) 101 102 it('excludes archived docs by default; includes when includeArchived is true', () => { 103 const archived = makeDoc({ id: 'archived', status: '4' }) 104 expect(filterMockedDocuments([archived], {})).toEqual([]) 105 expect(filterMockedDocuments([archived], { includeArchived: true })).toEqual([archived]) 106 }) 107 108 it('excludes obsolete docs by default; includes when includeObsolete is true', () => { 109 const obsolete = makeDoc({ id: 'o …(truncated, 2357 more chars) Now I have enough context to do a thorough analysis. Let me check one key thing — the visibility logic consistency between mapFiltersToSearchDocumentsParams.ts and filterMockedDocuments.ts: Tools used: - Bash: cd /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2181 && grep -n "DOCUMENT_VISIBILITY\|sharedWithAll\|notShared" packages/@doctolib/national-health-records/src/mocks/filterMockedDocument …(truncated, 109 more chars) Tool results: → result (ok): packages/@doctolib/national-health-records/src/mocks/filterMockedDocuments.ts:2:import { DOCUMENT_STATUS, DOCUMENT_VISIBILITY } from '@doctolib/dmp' packages/@doctolib/national-health-records/src/mocks/filterMockedDocuments.ts:60: // Build mask: sharedWithAll is always OR'd in so baseline docs are always included. packages/@doctolib/national-health-records/src/mocks/filterMockedDocuments.ts:62: let mask = DOCUMENT_VISIBILITY.sharedWithAll ?? 1 packages/@doctolib/national-health-records/src/moc …(truncated, 2027 more chars) I now have a complete picture. Here are my findings: --- Code Review Findings Bug 1 — matchesVisibility uses wrong semantics for hasVisibilityFilter (filterMockedDocuments.ts, lines 57–59) The hasVisibilityFilter check uses truthy evaluation of boolean flags: const hasVisibilityFilter = filters.includeMaskedToProfessionals || filters.includeInvisibleToPatient || filters.includeInvisibleToLegalReps When all three flags are false (explicitly set to false), hasVisibilityFilter is false, so the function returns true (no filtering). That is correct. However, the problem is that there is no guard against the case when a caller passes flags as false explicitly vs. when all are undefined. In both cases the result is the same, which is intentional — but this is not the bug. The actual issue: when hasVisibilityFilter is truthy (any flag is true), the mask always starts by OR-ing in DOCUMENT_VISIBILITY.sharedWithAll (value 1). A document with visibility: '4' (notSharedWithPractitioner) has bit-1 NOT set. The & mask check for that doc: 4 & (1 | 4) = 4 ≠ 0 → included. This is correct for includeMaskedToProfessionals: true. But when includeInvisibleToPatient: true and a doc has visibility: '4' (notSharedWithPractitioner, bit 4), the mask becomes 1 | 2 = 3. Then 4 & 3 = 0 → excluded. But the intent is that a doc masked to professionals (bit 4) should probably still be visible to a caller checking includeInvisibleToPatient. Whether this is correct depends on intent, but the mask-building logic is asymmetric: a doc can only pass if it matches the specific flag OR is sharedWithAll. A doc with visibility: '4' will be excluded when only includeInvisibleToPatient: true is set, because 4 & (1|2) = 0. That means a doc masked to professionals is completely invisible when the patient-visibility filter is applied but not the professional-visibility filter. This may be a mismodel of DMP semantics but is not clearly a new bug introduced by this PR (the visibility logic is new code, so it is in-scope). Bug 2 — matchesVisibility excludes the sharedWithAll doc with visibility: '1' when filter flags are false (not undefined) Wait — re-reading more carefully: the test at line 125–128 passes {} (undefined flags) and expects all docs to pass. When flags are explicitly false the same behavior should apply. No issue there. Real Bug — matchesCategory only checks codes[0] but nhrMockedDocuments has a doc with multiple codes (filterMockedDocuments.ts, line 22) function matchesCategory(doc: NhrDocument, filters: ServerFilters): boolean { if (!filters.typeCodes?.length) return true const code = doc.classification?.codes[0]?.code // ← only checks first code if (!code) return false return filters.typeCodes.includes(code) } nhrMockedDocuments.ts mock-040 has classification.codes with two entries: { system: 'dmp:classCode', code: 'Specialist report' }, { system: 'SNOMED', code: '371525003' }, Only codes[0] is checked. If a document's dmp:classCode were not in position 0, it would silently fail to match. In mock-040 the dmp:classCode happens to be at index 0, so in this specific test dataset it doesn't fail — but the matchesCategory function is supposed to simulate DMP server-side filtering which filters by dmp:classCode system, not array position. If any future document has the dmp:classCode entry at a non-zero index, the filter will produce wrong results. The fix should search all codes for the dmp:classCode system rather than hardcoding index 0. File: /packages/@doctolib/national-health-records/src/mocks/filterMockedDocuments.ts, line 22 Severity: Medium — currently masked by data layout, will silently misbehave for documents with multiple classification codes where dmp:classCode is not first. Bug 3 — mapStatusFilter returning undefined + mapFiltersToSearchDocumentsParams spreading it as status: undefined may conflict with NhrDocumentSearchParams type In mapFiltersToSearchDocumentsParams.ts (line 13): status: mapStatusFilter(appliedFilters), mapStatusFilter now returns string | undefined. When it returns undefined, status: undefined is included as an explicit key in the returned object. Whether the DMP API ignores explicit undefined fields vs. omitted fields depends on the serializer. This is likely fine in TypeScript (JSON.stringify omits undefined values), but it is a subtle difference from truly omitting the field (which would require ...mapStatusFilter(...) spread pattern like mapDateFilters uses). This is a pre-existing pattern issue (mapDateFilters returns a spread {}), but the inconsistency introduced here could cause issues if the API client does something like Object.keys(params).includes('status') to detect the field. This is a low-severity risk and likely fine in practice. --- Summary One clear bug worth flagging: matchesCategory only checks codes[0] instead of searching all codes for system === 'dmp:classCode' (filterMockedDocuments.ts, line 22). This makes category filtering fragile and incorrect for any document where the dmp:classCode entry is not the first code in the array. Mock document mock-040 already has two codes, meaning this is a real scenario in the existing test data, but it happens to have dmp:classCode at index 0 so tests pass accidentally. The correct implementation would be: const code = doc.classification?.codes.find(c => c.system === 'dmp:classCode')?.code All other changes (system string fix, generateNhrDocuments → filterMockedDocuments wiring, resolveDateType helper, mapStatusFilter returning undefined) look correct for their stated intent.