Project: doctolib-doctolib · Branch: .invalid · Mode:  · Model: claude-sonnet-4-6

Stats: 1 user messages, 9 tool calls, tools used: Bash, Read.

Conversation

Turn 1 — User

Analyze test coverage for the latest commit on branch fhir-2214_nhr-error-type-extension-and-modal-toast in ~/workspace/doctolib/worktrees/treatment-plan/fhir-2214.

Run git diff HEAD~1 HEAD to get the diff.

Context: The commit refactors error handling in NhrDocumentListModal from exception-based (try/catch) to status-based (checking result.status === 'error'). This is Approach A (error-as-data pattern).

Key files to review for test coverage:
- packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.test.tsx — the main test file
- packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.tsx — the component under test

What changed in the component: searchDocuments is awaited, then its result.status is checked. If 'error', sets empty state and fires addToast. Otherwise sets rows/total from result data.

Look for: gaps in error path coverage, missing edge cases (e.g. non-'error' non-'success' statuses like 'partial success', 'skipped'), whether the toast fallback message path is tested, and whether the loading state transitions are adequately covered.

Turn 1 — Assistant

I'll analyze the test coverage for this commit. Let me start by examining the diff and the key files.

Tools used:

- Bash: cd ~/workspace/doctolib/worktrees/treatment-plan/fhir-2214 && git diff HEAD~1 HEAD

Tools used:

- Read: /Users/USER/workspace/doctolib/worktrees/treatment-plan/fhir-2214/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.tsx

Tool results:

  → result (ok): diff --git a/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.stories.tsx b/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.stories.tsx
index 2491d03d9..ef2855c42 100644
--- a/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.stories.tsx
+++ b/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.stories.tsx
@@ 
…(truncated, 18735 more chars)

Tool results:

  → result (ok): 1	import { Modal } from '@doctolib/design-system/Modal'
2	import { ModalBody } from '@doctolib/design-system/ModalBody'
3	import type { NhrDocument } from '@doctolib/tpf-national-health-records'
4	import { useEffect, useMemo, useState } from 'react'
5	
6	import { addToast } from '../../business/toast'
7	import type { ActionConfig, ColumnConfig, FilterConfig, NhrPatientInfo, SearchDocumentsFn } from '../../types'
8	import { NhrDocumentList } from '../NhrDocumentList/NhrDocumentList'
9	
10	export 
…(truncated, 3274 more chars)

Now let me read the full test file to understand the existing test coverage in detail.

Tools used:

- Read: /Users/USER/workspace/doctolib/worktrees/treatment-plan/fhir-2214/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.test.tsx

Tool results:

  → result (ok): 1	import type { DesignSystemIconComponent } from '@doctolib/design-system-icons'
2	import { setup } from '@doctolib/test-helpers'
3	import { screen, waitFor } from '@testing-library/react'
4	import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
5	
6	import { createMoreActionsAction } from '../../business/factories/actions/createMoreActionsAction'
7	import { mockColumns, mockPatientInfo } from '../../test-helpers'
8	import { nhrDocumentFixtureBuilder } from '../../test-helpers/
…(truncated, 12769 more chars)

Tools used:

- Read: /Users/USER/workspace/doctolib/worktrees/treatment-plan/fhir-2214/packages/tpf-national-health-records/src/business/types/api.ts

Tool results:

  → result (ok): 1	import type { DateRange, NhrDocument, NhrDocumentFile } from '#business/types/document'
2	import type { NhrPatientInformation } from '#business/types/participant'
3	
4	export type NhrResultStatus = 'success' | 'skipped' | 'error' | 'partial success'
5	
6	export type NhrResult = {
7	  status: NhrResultStatus
8	  message?: string
9	  code?: string
10	}
11	
12	// Document Search
13	export type NhrDocumentSearchParams = {
14	  formats?: string[]
15	  categories?: string[]
16	  status?: string
17	 
…(truncated, 1958 more chars)

Now let me look at how SearchDocumentsFn is typed in the UI package and what callers pass as searchDocuments to understand the full contract.

Tools used:

- Read: /Users/USER/workspace/doctolib/worktrees/treatment-plan/fhir-2214/packages/tpf-national-health-records-ui/src/types.ts

Tool results:

  → result (ok): 1	import type { DesignSystemIconComponent } from '@doctolib/design-system-icons'
2	import type { NhrDocument, NhrResultStatus } from '@doctolib/tpf-national-health-records'
3	import type { ReactNode } from 'react'
4	
5	export type NhrPatientInfo = {
6	  fullName: string
7	  gender: 'male' | 'female' | 'other'
8	  birthDate: string
9	  age: number
10	}
11	
12	export type ColumnConfig = {
13	  key: string
14	  header: string
15	  sortable: boolean
16	  / Only required when sortable: true. Return
…(truncated, 3486 more chars)

Tools used:

- Bash: cd ~/workspace/doctolib/worktrees/treatment-plan/fhir-2214 && grep -r "searchDocuments" packages/ --include=".ts" --include=".tsx" -l

Tool results:

  → result (ok): packages/tpf-national-health-records-ui/src/components/NhrEntryPoint/NhrEntryPoint.tsx
packages/tpf-national-health-records-ui/src/components/NhrEntryPoint/NhrEntryPoint.test.tsx
packages/tpf-national-health-records-ui/src/components/NhrEntryPoint/NhrEntryPoint.stories.tsx
packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.stories.tsx
packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.test.tsx
packages/tp
…(truncated, 343 more chars)

Tools used:

- Read: /Users/USER/workspace/doctolib/worktrees/treatment-plan/fhir-2214/packages/tpf-national-health-records/src/business/types/service.ts

Tool results:

  → result (ok): 1	import type {
2	  NhrDocumentContentResult,
3	  NhrDocumentMetadataUpdateProps,
4	  NhrDocumentMetadataUpdateResult,
5	  NhrDocumentSearchProps,
6	  NhrDocumentSearchResult,
7	  NhrGetDocumentsContentProps,
8	} from '#business/types/api'
9	
10	export type CountryCode = 'fr' | 'mock'
11	
12	export type NhrService = {
13	  searchDocuments(props: NhrDocumentSearchProps): Promise<NhrDocumentSearchResult>
14	  getDocumentsContent(props: NhrGetDocumentsContentProps): Promise<NhrDocumentContentResult
…(truncated, 452 more chars)

Tools used:

- Read: /Users/USER/workspace/doctolib/worktrees/treatment-plan/fhir-2214/packages/tpf-national-health-records/src/business/systems/dmp/services/dmp-nhr-service.ts

Tool results:

  → result (ok): 1	import type {
2	  NhrDocumentContentResult,
3	  NhrDocumentMetadataUpdateProps,
4	  NhrDocumentMetadataUpdateResult,
5	  NhrDocumentSearchProps,
6	  NhrDocumentSearchResult,
7	  NhrGetDocumentsContentProps,
8	} from '#business/types/api'
9	import type { NhrAdapter, NhrService } from '#business/types/service'
10	
11	export class DmpNhrService implements NhrService {
12	  readonly country = 'fr'
13	
14	  constructor(readonly nhrAdapter: NhrAdapter) {}
15	
16	  searchDocuments = (props: NhrDocume
…(truncated, 435 more chars)

Now I have a complete picture of the code. Let me examine one more thing — how the NhrEntryPoint test handles searchDocuments to see if any integration-level coverage exists there.

Tools used:

- Read: /Users/USER/workspace/doctolib/worktrees/treatment-plan/fhir-2214/packages/tpf-national-health-records-ui/src/components/NhrEntryPoint/NhrEntryPoint.test.tsx

Tool results:

  → result (ok): 1	import { t } from '#i18n'
2	import { setup } from '@doctolib/test-helpers'
3	import { screen } from '@testing-library/react'
4	import { describe, expect, it, vi } from 'vitest'
5	
6	import { mockColumns, mockPatientInfo } from '../../test-helpers'
7	import { NhrEntryPoint } from './NhrEntryPoint'
8	
9	describe('NhrEntryPoint', () => {
10	  it('should render the button if show_ins_features flag in patient is true and status is enabled', () => {
11	    setup(
12	      <NhrEntryPoint
13	        p
…(truncated, 2240 more chars)

I now have a thorough understanding of the full picture. Here is the analysis.

---

Summary

The test suite is well-structured and covers the core behavioral contract of the refactored component. The migration from exception-based to status-based error handling is tested directly, and the important paths — error with message, error without message (fallback), loading state teardown on error, and repeat-error-on-filter-change — are all present. However, there are meaningful gaps that could silently mask real production failures.

---

Critical Gaps

1. Non-'error', non-'success' statuses are not tested — 'skipped' and 'partial success'
Criticality: 8

The NhrResultStatus type (in /Users/USER/workspace/doctolib/worktrees/treatment-plan/fhir-2214/packages/tpf-national-health-records/src/business/types/api.ts) is a discriminated union of four values: 'success' | 'skipped' | 'error' | 'partial success'. The component's else branch in NhrDocumentListModal.tsx handles every non-'error' status identically — it trusts result.data and result.total unconditionally. No test verifies that 'skipped' or 'partial success' results are rendered correctly (or at all).

This is the highest-priority gap because the contract is actively ambiguous: a 'partial success' response may arrive with an empty data array, or with a partial set plus a message describing what failed — yet the component silently renders it as a normal success and fires no toast. A future adapter change that starts returning 'partial success' would go completely unnoticed by tests.

Tests to add:
- status: 'skipped' — what should the UI show? Currently it silently sets rows: result.data and isLoading: false. Is that the intended behavior?
- status: 'partial success' — same question. Documents rendered but no warning? The UX decision needs to be tested and locked down.

Example failure this would catch: A DMP adapter returns { status: 'partial success', data: [], message: 'One source timed out' }. The component silently shows an empty table with no indication that results may be incomplete.

---

2. The isLoading: true intermediate state is not verified for the success path
Criticality: 7

The component unconditionally sets isLoading: true at the start of each useEffect run (line 44 of NhrDocumentListModal.tsx). Multiple tests use a never-resolving searchDocuments to keep the component in that loading state — but no test asserts that a spinner/skeleton is visible while loading, then becomes invisible after a successful fetch resolves. The "stop loading" assertion only exists for the error path (should stop loading and clear rows when searchDocuments returns an error status). A refactor that accidentally removed the isLoading: false transition in the success branch would not be caught.

Test to add: Assert that a loading indicator is present before searchDocuments resolves, then absent (and rows visible) after it resolves with status: 'success'.

Example failure: Someone removes isLoading: false from the success else branch. The spinner never disappears. Zero existing tests catch this.

---

3. No test covers re-fetch loading state on filter change for a successful second call
Criticality: 6

The filter-change test (should re-fetch documents with applied filters when filters are submitted) only asserts that searchDocuments was called the correct number of times with the correct arguments. It does not verify that isLoading is reset to true for the second fetch, nor that it returns to false after the second fetch resolves. The isLoading reset is explicit in the useEffect at setDocumentsState((prev) => ({ ...prev, isLoading: true })) — but the full cycle is only indirectly exercised.

---

Important Improvements

4. NhrEntryPoint.test.tsx has stale searchDocuments mocks that are now missing status
Criticality: 6

Three tests in /Users/USER/workspace/doctolib/worktrees/treatment-plan/fhir-2214/packages/tpf-national-health-records-ui/src/components/NhrEntryPoint/NhrEntryPoint.test.tsx still use vi.fn().mockResolvedValue({ data: [] }) without a status field. One uses mockReturnValue(new Promise(() => {})) which is fine. The three missing-status mocks violate the SearchDocumentsFn contract as tightened in this commit — they would produce result.status === undefined, which falls into the else branch and silently succeeds, but they do it in a way that is inconsistent with the declared type. This is a maintainability issue: it signals that the type enforcement isn't propagating to all test sites. These should be { data: [], status: 'success' as const } to match the pattern established in NhrDocumentListModal.test.tsx.

This is not a behavioral bug today (the modal isn't opened in those three tests), but it erodes test clarity and leaves a false signal: a future developer reading these tests might conclude that status is optional.

---

5. The code field on the error result is never asserted
Criticality: 4

SearchDocumentsFn now includes an optional code field in the resolved value, and NhrResult in the backend type carries code?: string. The component ignores code entirely — it only reads status and message. No test verifies that a result with { status: 'error', code: 'DMP_503', message: '...' } behaves identically to one without code. This is a minor gap: the behavior is correct by design since code is intentionally not used in the component, but the absence of any test asserting this means the next engineer might add code-based branching without realizing tests won't catch a regression in the existing path. A single negative test confirming code is ignored would document the intent.

---

Test Quality Issues

6. The fallback-message test previously conflated "non-Error thrown" with "no message in result"

The old test was named should use the fixed error title as message fallback when searchDocuments throws a non-Error. The new name should use the fixed error title as message fallback when no message is provided is correct and better, and the mock now returns { data: [], status: 'error' } with no message field. This is a good behavioral improvement. However, the test now proves only that result.message being undefined triggers the fallback via ??. It does not verify that result.message being an empty string '' also falls back correctly — '' ?? errorTitle evaluates to '' (empty string), not errorTitle, because ?? checks nullish, not falsy. If an adapter ever returns { status: 'error', message: '' }, the toast would show an empty message. This is worth a note:

Test to add (criticality 5): { status: 'error', message: '' } — assert that the toast shows either the fallback title or a non-empty message. Documents the intended behavior of the ?? operator choice vs. ||.

---

Positive Observations

- The four error-path tests are well-focused on behavior (what toast fires, what state is set) rather than implementation details. They would survive a refactor of the internal fetchDocuments function.
- The beforeEach pattern with a never-resolving promise is an excellent guard against flaky async state updates in tests that only care about the synchronous rendering behavior.
- The "no toast on success" negative test is present and correct — this is often overlooked and is valuable for preventing accidental toast regressions.
- Renaming tests away from exception-centric names (throws an Error, throws a non-Error) to result-centric names correctly documents the new contract and will be clear to future reviewers.
- The stories files were updated in sync with the type change, which keeps the storybook from becoming a source of type-incorrect examples.