Project: workspace-doctolib · Branch: HEAD · Mode: ` · **Model:**claude-sonnet-4-6`
Stats: 1 user messages, 17 tool calls, tools used: Bash, Read.
Conversation
Turn 1 — User
Analyze test coverage quality for the doctolib monolith PR https://github.com/doctolib/doctolib/pull/241176
Worktree: /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/
Changed files under packages/@doctolib/national-health-records/src/variants/:
- types.ts — added visibleToAll?: boolean to DmpServerFilters
- dmp/factories/filtersFactory.ts — prepended "Visible to all" with isExclusive: true
- dmp/mappers/mapFiltersToSearchDocumentsParams.ts — early-return for visibleToAll: true
- dmp/factories/filtersFactory.test.ts — updated tests for 4 options and first-option assertion
- dmp/mappers/mapFiltersToSearchDocumentsParams.test.ts — 2 new tests for visibleToAll
Get the diff:
cd /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter && git diff HEAD~1...HEAD
Evaluate whether the test coverage is sufficient and identify any critical gaps (criticality >= 7).
Turn 1 — Assistant
I'll analyze the PR's changes and test coverage. Let me start by examining the diff and relevant files.
Tools used:
Bash:cd /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter && git diff HEAD~1...HEAD
Tool results:
→ result (ok): diff --git a/packages/@doctolib/national-health-records/src/variants/dmp/factories/filtersFactory.test.ts b/packages/@doctolib/national-health-records/src/variants/dmp/factories/filtersFactory.test.ts index 529b56677f0e..c0418b93d8a5 100644 --- a/packages/@doctolib/national-health-records/src/variants/dmp/factories/filtersFactory.test.ts +++ b/packages/@doctolib/national-health-records/src/variants/dmp/factories/filtersFactory.test.ts @@ -91,17 +91,17 @@ describe('filtersFactory', () => { ) …(truncated, 4664 more chars)
Now let me read the full source files to understand the complete context around the changes.
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib/national-health-records/src/variants/dmp/mappers/mapFiltersToSearchDocumentsParams.ts
Tool results:
→ result (ok): 1 / eslint-disable no-bitwise /
2 import type { NhrDocumentSearchParams } from '@doctolib/tpf-national-health-records'
3 import { DOCUMENT_STATUS, DOCUMENT_VISIBILITY } from '@doctolib/dmp'
4 import type { ServerFilters } from '../../types'
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 NhrDocumentSearchParams.
8 export function mapFiltersToSearchDocumentsParam
…(truncated, 2888 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib/national-health-records/src/variants/dmp/mappers/mapFiltersToSearchDocumentsParams.test.ts
Tool results:
→ result (ok): 1 import { mapFiltersToSearchDocumentsParams } from './mapFiltersToSearchDocumentsParams'
2
3 jest.mock('@doctolib/dmp', () => ({
4 DOCUMENT_STATUS: { validated: 1, obsolete: 2, archived: 4 },
5 DOCUMENT_VISIBILITY: {
6 sharedWithAll: 1,
7 notSharedWithPatient: 2,
8 notSharedWithPractitioner: 4,
9 notSharedWithLegalRepresentative: 8,
10 },
11 }))
12
13 describe('mapFiltersToSearchDocumentsParams', () => {
14 describe('categories', () => {
15 it('maps typeCodes to ca
…(truncated, 6868 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib/national-health-records/src/variants/dmp/factories/filtersFactory.ts
Tool results:
→ result (ok): 1 import {
2 type FilterConfig,
3 createNhrDateSelectionFilterConfig,
4 createNhrDocumentCategoryFilterConfig,
5 createNhrDocumentStatusFilterConfig,
6 createNhrDocumentVisibilityFilterConfig,
7 } from '@doctolib/tpf-national-health-records-ui'
8 import { DMP_CATEGORY_OPTIONS } from './dmpCategoryOptions'
9
10 type FiltersOptions = {
11 lastAccessDate?: string
12 }
13
14 export function filtersFactory({ lastAccessDate = '' }: FiltersOptions = {}): FilterConfig[] {
15 return [
16
…(truncated, 1438 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib/national-health-records/src/variants/dmp/factories/filtersFactory.test.ts
Tool results:
→ result (ok): 1 import { filtersFactory } from './filtersFactory'
2
3 jest.mock('./dmpCategoryOptions', () => ({
4 DMP_CATEGORY_OPTIONS: [
5 { value: '11488-4', label: 'CR ou fiche de consultation ou de visite' },
6 { value: 'CERT_DECL', label: 'Certificat, déclaration' },
7 ],
8 }))
9
10 jest.mock('@doctolib/tpf-national-health-records-ui', () => ({
11 createNhrDocumentCategoryFilterConfig: jest.fn(),
12 createNhrDateSelectionFilterConfig: jest.fn(),
13 createNhrDocumentStatusFilterConfig
…(truncated, 5247 more chars)
Now let me look at the DOCUMENT_VISIBILITY constant and the createNhrDocumentVisibilityFilterConfig function signature to understand the isExclusive contract.
Tools used:
Bash:find /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib -type f -name "*.ts" | xargs grep -l "isExclusive" 2>/dev/null | head -20
Tools used:
Bash:find /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib -type f -name "*.ts" | xargs grep -l "DOCUMENT_VISIBILITY" 2>/dev/null | head -20
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib/national-health-records/src/variants/dmp/factories/filtersFactory.ts /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib/national-health-records/src/variants/dmp/factories/filtersFactory.test.ts
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib/dmp/src/index.ts /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib/national-health-records/src/mocks/filterMockedDocuments.ts /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts /Users/USER/workspace/doctolib/worktrees/docto …(truncated, 1252 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib/dmp/src/business/FrDmpConstants.ts
Tool results:
→ result (ok): 1 export const FR_DMP_DOCUMENT_FORMAT = {
2 jepg: 3,
3 tiff: 4,
4 pdf: 5,
5 medicalSynthesis: 6,
6 }
7
8 export const STATUS_OK = 'OK'
9
10 export const SESSION_TIMEOUT_SECONDS = 4 * 60 * 60
11
12 export const DOCUMENT_VISIBILITY: Record<string, number> = {
13 sharedWithAll: 1,
14 notSharedWithPatient: 2,
15 notSharedWithPractitioner: 4,
16 notSharedWithLegalRepresentative: 8,
17 sharedOnlyWithPractitioner: 10,
18 sharedOnlyWithPatient: 12,
19 }
20
21 export const DMP_BIO
…(truncated, 659 more chars)
Tools used:
Bash:find /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib -type f -name "*.ts" | xargs grep -l "createNhrDocumentVisibilityFilterConfig" 2>/dev …(truncated, 16 more chars)
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib/national-health-records/src/variants/dmp/factories/filtersFactory.ts /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib/national-health-records/src/variants/dmp/factories/filtersFactory.test.ts
Tools used:
Bash:find /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages -type f -name "*.ts" | xargs grep -l "createNhrDocumentVisibilityFilterConfig" 2>/dev/null
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/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, 3010 more chars)
Tool results:
→ result (ERROR): Exit code 1 /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib/national-health-records/src/variants/dmp/factories/filtersFactory.ts /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib/national-health-records/src/variants/dmp/factories/filtersFactory.test.ts
Tools used:
Bash:find /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib/national-health-records/src/mocks -type f -name "*.test.ts" 2>/dev/null
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib/national-health-records/src/mocks/filterMockedDocuments.test.ts /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib/national-health-records/src/mocks/generateNhrDocuments.test.ts
Tools used:
Bash:find /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib/national-health-records/src -type f -name "filterMockedDocuments*" 2>/dev/null
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib/national-health-records/src/mocks/filterMockedDocuments.test.ts /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib/national-health-records/src/mocks/filterMockedDocuments.ts
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib/national-health-records/src/mocks/filterMockedDocuments.test.ts
Tool results:
→ result (ok): 1 import type { NhrDocument } from '@doctolib/tpf-national-health-records'
2 import { filterMockedDocuments } from './filterMockedDocuments'
3
4 const makeDoc = (overrides: Partial<NhrDocument> = {}): NhrDocument => ({
5 id: 'doc-1',
6 title: 'Test document',
7 ...overrides,
8 })
9
10 describe('filterMockedDocuments', () => {
11 describe('category filter', () => {
12 it('returns all documents when no filters are set', () => {
13 const docs = [makeDoc({ id: 'a' }), makeDoc({ i
…(truncated, 9481 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/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<string, unknown>) => Promise<
…(truncated, 848 more chars)
Now I have a complete picture of the changes and the test coverage. Let me verify one more thing — the filterMockedDocuments.ts which handles visibleToAll at the mock layer.
Tools used:
Bash:grep -n "visibleToAll" /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib/national-health-records/src/mocks/filterMockedDocuments.ts 2>/dev/n …(truncated, 3 more chars)
Tool results:
→ result (ok): (Bash completed with no output)
Tools used:
Bash:grep -rn "visibleToAll" /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib/national-health-records/src/ 2>/dev/null
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib/national-health-records/src/variants/types.ts:25: visibleToAll?: boolean /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib/national-health-records/src/variants/dmp/factories/filtersFactory.ts:44: { filterKey: 'visibleToAll', label: 'Visible to all', isExclusive: true }, /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_a …(truncated, 1560 more chars)
Now I have everything needed. Here is the full analysis.
Summary
The PR adds a "Visible to all" filter option (visibleToAll: boolean) to the DMP visibility filter panel. The change touches three logical layers: the type definition, the UI filter factory, and the API parameter mapper. Tests were added or updated in both test files that correspond to those layers. Overall the coverage is solid for the happy paths, but there is one critical gap and two notable omissions.
Critical Gaps
1. filterMockedDocuments does not handle visibleToAll — criticality 9
File: /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib/national-health-records/src/mocks/filterMockedDocuments.ts
matchesVisibility in filterMockedDocuments.ts has no branch for visibleToAll. The real API path (mapVisibilityFilter) was updated correctly, but the mock path was not touched at all. The two paths are supposed to mirror each other — the comment at the top of filterMockedDocuments.ts explicitly says: "Used so that the mock path in useSearchDocuments respects filter changes the same way the real API does."
Because visibleToAll is absent from the mock, when a developer uses the mock mode (e.g., local dev or Storybook), activating the "Visible to all" filter will produce no visible change in the document list. The real API correctly sends visibility=1 (sharedWithAll), so the product behaves differently in production versus mock mode. This discrepancy can mask bugs and causes confusing local development behavior.
What a test would catch: A test calling filterMockedDocuments(docs, { visibleToAll: true }) would fail immediately today because the filter returns the same result as the empty-filter case — it has no awareness of visibleToAll.
Tests to add in filterMockedDocuments.test.ts:
- visibleToAll: true returns only docs whose visibility is '1' (sharedWithAll) and docs with undefined visibility.
- visibleToAll: true with mixed visibility values excludes docs with visibility: '2', '4', '8', etc.
- visibleToAll: true takes priority over other visibility flags (parallel to the mapper test).
2. visibleToAll: false is not tested in the mapper — criticality 7
File: /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib/national-health-records/src/variants/dmp/mappers/mapFiltersToSearchDocumentsParams.ts
The early-return guard is if (filters.visibleToAll). This means visibleToAll: false (explicitly set to false) falls through to the normal bitmask logic, which is correct. But there is no test for { visibleToAll: false, includeInvisibleToPatient: true } to confirm the other visibility flags still work when visibleToAll is explicitly false. If someone accidentally changed the guard to if (filters.visibleToAll !== undefined) during a refactor, the existing tests would still pass, and the regression would be silent.
Test to add in mapFiltersToSearchDocumentsParams.test.ts:
it('visibleToAll: false does not suppress other visibility flags', () => {
const result = mapFiltersToSearchDocumentsParams({ visibleToAll: false, includeInvisibleToPatient: true })
expect(result.visibility).toBe('2') // notSharedWithPatient, not sharedWithAll
})
Important Improvements
3. filtersFactory.test.ts does not assert the order of the remaining three visibility options — criticality 5
File: /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2222_add-visible-to-all-filter/packages/@doctolib/national-health-records/src/variants/dmp/factories/filtersFactory.test.ts
The test at line 100 asserts that visibilityModes[0] is the "Visible to all" option with isExclusive: true, which is important. However, individual tests for options at index 1, 2, and 3 use expect.arrayContaining, which is order-agnostic. This means a developer could swap the position of "Hidden from patient" and "Hidden from legal guardians" in filtersFactory.ts without any test failing, even though the UI order is a meaningful product decision. This is low-urgency given the isExclusive: true option's position is already guarded, but worth noting.
4. No test for visibleToAll: true combined with status or category filters in the mapper — criticality 5
The "combined filters" describe block at line 144 of mapFiltersToSearchDocumentsParams.test.ts does not include visibleToAll: true. If visibleToAll were accidentally made to suppress the status or category fields as well (e.g., through an early return on the whole function rather than just the visibility sub-function), no existing test would catch it.
Test Quality Issues
None of the existing tests appear to over-specify implementation details. The mock in filtersFactory.test.ts correctly tests the configuration arguments passed to createNhrDocumentVisibilityFilterConfig rather than the rendered output, which is appropriate. The mapper tests use named bitmask constants with inline comments explaining the arithmetic, which is good.
The ?? 1 fallback in mapVisibilityFilter (String(DOCUMENT_VISIBILITY.sharedWithAll ?? 1)) is dead code because DOCUMENT_VISIBILITY is typed as Record<string, number> and sharedWithAll will always be a number when the real constant is used. The mock in the test file always defines sharedWithAll: 1, so this branch is never exercised. This is not a test quality issue per se, but the fallback creates an illusion of defensive coding that has no test coverage and could silently mask a misconfigured import.
Positive Observations
- The two new mapper tests correctly use the real mock values (matching
sharedWithAll: 1) and the priority test with a conflicting flag is a well-chosen scenario. - The factory test correctly checks
isExclusive: trueon the new option — this is the property that drives the UI mutual-exclusivity behavior, so testing it explicitly is the right call. - The existing
mapFiltersToSearchDocumentsParams.test.tssuite is thorough on all pre-existing paths and the new tests integrate cleanly into the established pattern. - The type addition in
types.tsplacingvisibleToAllbetweenincludeObsoleteandincludeInvisibleToPatientgroups it correctly with the DMP-specific filters, and the optional?prevents breaking changes to existing call sites.