Project: doctolib-doctolib · Branch: .invalid · Mode: ` · **Model:**claude-sonnet-4-6`
Stats: 1 user messages, 17 tool calls, tools used: Bash, Read.
Conversation
Turn 1 — User
Analyze the test coverage in the latest fix commit on fhir-2214_nhr-error-type-extension-and-modal-toast branch in /Users/USER/workspace/doctolib/treatment-plan.
Run: cd ~/workspace/doctolib/treatment-plan && git diff HEAD~1 HEAD -- packages/tpf-national-health-records-ui/src/ to see what changed.
The main test file is: packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.test.tsx
Context:
- The feature adds error handling + toast notifications to NhrDocumentListModal when searchDocuments throws
- Previous feedback asked to: assert title field, assert message content for non-Error fallback, verify rows/cells are empty after error
Changes made to tests:
1. Error toast test now asserts title: expect.any(String) in addition to message and uiStyle
2. Non-Error fallback test now asserts title, message, uiStyle via objectContaining AND verifies message === title via vi.mocked(addToast).mock.calls[0]?.[0]
3. Stop-loading test now has queryAllByRole('cell') inside waitFor (waits for skeleton to disappear) to verify empty body
Are these test improvements sufficient? Are there any remaining critical gaps in coverage?
Turn 1 — Assistant
Let me examine the actual code changes and tests carefully.
Tools used:
Bash:cd ~/workspace/doctolib/treatment-plan && git log --oneline -5 fhir-2214_nhr-error-type-extension-and-modal-toast 2>/dev/null || git log --oneline -5 origin/fhir-2214_nhr-error-type-extension-and-moda …(truncated, 19 more chars)
Tools used:
Bash:cd ~/workspace/doctolib/treatment-plan && git diff HEAD~1 HEAD -- packages/tpf-national-health-records-ui/src/ 2>/dev/null | head -400
Tool results:
→ result (ok): 1db6d600a fix(FHIR-2214): address code review feedback on NHR error toast edd330ab5 feat(FHIR-2214): add NHR error type extension and modal error toast 0ef9d99fc feat(ACAI-4219): add GET /bundles/{bundle_id}/templates endpoint (#2972)
Tool results:
→ result (ok): diff --git a/packages/tpf-national-health-records-ui/src/business/toast.ts b/packages/tpf-national-health-records-ui/src/business/toast.ts index 15715cdee..efe8018e6 100644 --- a/packages/tpf-national-health-records-ui/src/business/toast.ts +++ b/packages/tpf-national-health-records-ui/src/business/toast.ts @@ -1,11 +1,12 @@ -import { GLOBAL_PUBSUB_REGISTRY } from "@doctolib/upfront-core"; +import { GLOBAL_PUBSUB_REGISTRY } from '@doctolib/upfront-core'
type ToastProps = { - message: string; …(truncated, 13345 more chars)
Tools used:
Bash:cd ~/workspace/doctolib/treatment-plan && git diff HEAD~1 HEAD -- packages/tpf-national-health-records-ui/src/ 2>/dev/null | tail -400
Tool results:
→ result (ok): + await user.click(screen.getByRole('button', { name: /Filters/ }))
// Change a filter value then apply
- await user.click(screen.getByRole("button", { name: "Set Category" }));
- await user.click(screen.getByRole("button", { name: "Apply filters" }));
- await user.click(screen.getByRole('button', { name: 'Set Category' }))
-
await user.click(screen.getByRole('button', { name: 'Apply filters' }))
-
expect(searchDocuments).toHaveBeenCalledTimes(2);
- expect(searchDoc …(truncated, 13453 more chars)
Now let me read the full test file to understand the complete picture, and also look at the source component for anything the tests might be missing.
Tools used:
Read:/Users/USER/workspace/doctolib/treatment-plan/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 } from '@testing-library/react'
4 import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
5
6 import { mockColumns, mockPatientInfo } from '../../test-helpers'
7 import { nhrDocumentFixtureBuilder } from '../../test-helpers/fixtures/nhrDocumentFixtureBuilder'
8 import type { ActionConfig, FilterConfig } from '../../types'
9 import {
…(truncated, 11230 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.tsx
Tool results:
→ result (ok): 1 import { t } from '#i18n'
2 import { Modal } from '@doctolib/design-system/Modal'
3 import { ModalBody } from '@doctolib/design-system/ModalBody'
4 import type { NhrDocument } from '@doctolib/tpf-national-health-records'
5 import { useEffect, useMemo, useState } from 'react'
6
7 import { addToast } from '../../business/toast'
8 import type { ActionConfig, ColumnConfig, FilterConfig, NhrPatientInfo, SearchDocumentsFn } from '../../types'
9 import { NhrDocumentList } from '../NhrDocumentList/Nh
…(truncated, 2776 more chars)
Now let me look at the toast.ts change more carefully and the business logic around the non-Error fallback, plus check if there are toast.ts tests.
Tools used:
Bash:find /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/business -type f | sort
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/business/factories/actions/createMoreActionsAction.test.ts /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/business/factories/actions/createMoreActionsAction.ts /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/business/factories/actions/createViewDocumentAction.test.ts /Users/USER/workspace/doctolib/treatment-plan/packages/tp …(truncated, 3847 more chars)
Tools used:
Bash:cd ~/workspace/doctolib/treatment-plan && git log --oneline -3 fhir-2214_nhr-error-type-extension-and-modal-toast -- packages/tpf-national-health-records-ui/src/business/toast.ts
Tool results:
→ result (ok): 1db6d600a fix(FHIR-2214): address code review feedback on NHR error toast edd330ab5 feat(FHIR-2214): add NHR error type extension and modal error toast
Tools used:
Read:/Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/business/toast.ts
Tool results:
→ result (ok): 1 import { GLOBAL_PUBSUB_REGISTRY } from '@doctolib/upfront-core'
2
3 type ToastProps = {
4 message: React.ReactNode
5 title: string
6 uiStyle: 'info' | 'success' | 'error' | 'warning'
7 action?: { message: string; onClick: () => void }
8 }
9
10 export function addToast(props: ToastProps) {
11 GLOBAL_PUBSUB_REGISTRY.dispatch({ type: 'toasts:add', payload: props })
12 }
13
Tools used:
Bash:cd ~/workspace/doctolib/treatment-plan && git show edd330ab5 -- packages/tpf-national-health-records-ui/src/ 2>/dev/null | grep "^diff\|^---\|^+++" | head -30
Tool results:
→ result (ok): diff --git a/packages/tpf-national-health-records-ui/src/business/toast.ts b/packages/tpf-national-health-records-ui/src/business/toast.ts --- /dev/null +++ b/packages/tpf-national-health-records-ui/src/business/toast.ts diff --git a/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.test.tsx b/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.test.tsx --- a/packages/tpf-national-health-records-ui/src/co …(truncated, 1384 more chars)
Tools used:
Bash:find /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/locales -type f | xargs grep -l "errors" 2>/dev/null
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/locales/fr/tpf_national_health_records_ui.json /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/locales/en/tpf_national_health_records_ui.json
Tools used:
Bash:grep -r "errors" /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/locales/ 2>/dev/null
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/locales/fr/tpf_national_health_records_ui.json: "errors.search.title": "Impossible de charger les documents DMP", /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/locales/en/tpf_national_health_records_ui.json: "errors.search.title": "Unable to load DMP documents",
Tools used:
Bash:find /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList -type f | sort
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentCategoryCell/index.ts /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentCategoryCell/NhrDocumentCategoryCell.stories.tsx /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentCategoryCell/NhrDocumentCategoryCell.test.tsx …(truncated, 5399 more chars)
Tools used:
Bash:grep -n "isLoading\|skeleton\|cell\|rows\|data" /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentList.tsx 2>/dev/nul …(truncated, 12 more chars)
Tool results:
→ result (ok): 16: data?: NhrDocument[] 17: isLoading?: boolean 23: /* Total number of results from the server. When provided, takes precedence over data.length / 31: data, 32: isLoading = true, 41: const pagedData = useMemo(() => getPagedData(data, currentPage), [data, currentPage]) 54: const resultCount = resultCountProp ?? data?.length 68: data-testid='action-loading-overlay' 75: rows={pagedData} 76: isLoading={isLoading} 79: total={resultCountProp ?? data?.len …(truncated, 9 more chars)
Now I have a complete picture. Let me also check the specific behavior around total: undefined in the error state vs total: 0 in the old state, as this was changed in the production code.
Tools used:
Bash:grep -n "total\|resultCount\|undefined\|0 results\|results" /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentList.ts …(truncated, 12 more chars)
Tool results:
→ result (ok): 15: // Data fetching to be implemented in future work; leave undefined for now for Storybook 23: /* Total number of results from the server. When provided, takes precedence over data.length / 24: resultCount?: number 37: resultCount: resultCountProp, 54: const resultCount = resultCountProp ?? data?.length 62: resultCount={resultCount} 79: total={resultCountProp ?? data?.length ?? 0}
Tools used:
Read:/Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentList.tsx
Tool results:
→ result (ok): 1 import { getPagedData } from '#utils/nhrPaginationHelpers'
2 import { SpinnerLoader } from '@doctolib/design-system/SpinnerLoader'
3 import type { NhrDocument } from '@doctolib/tpf-national-health-records'
4 import { useMemo, useState } from 'react'
5
6 import type { ActionConfig, ColumnConfig, FilterConfig, NhrPatientInfo } from '../../types'
7 import { NhrDocumentListFilterPanel } from '../NhrDocumentListFilterPanel/NhrDocumentListFilterPanel'
8 import { NhrDocumentListHeader } from '../Nhr
…(truncated, 3113 more chars)
Now I have everything I need to provide a thorough analysis. Let me also check one more detail — the production code change from total: 0 to total: undefined in the error case, as that's a behavioral change that isn't directly tested.
Tools used:
Bash:cd ~/workspace/doctolib/treatment-plan && git show edd330ab5 -- packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.tsx 2>/dev/null | grep "^+" | grep -v " …(truncated, 16 more chars)
Tool results:
→ result (ok): +import { Modal } from "@doctolib/design-system/Modal"; +import { ModalBody } from "@doctolib/design-system/ModalBody"; +import type { NhrDocument } from "@doctolib/tpf-national-health-records"; +import { useEffect, useMemo, useState } from "react"; +import { addToast } from "../../business/toast"; +import type { + ActionConfig, + ColumnConfig, + FilterConfig, + NhrPatientInfo, + SearchDocumentsFn, +} from "../../types"; +import { NhrDocumentList } from "../NhrDocumentList/NhrDocumentList"; …(truncated, 174 more chars)
Tools used:
Bash:cd ~/workspace/doctolib/treatment-plan && git show edd330ab5 -- packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.tsx 2>/dev/null | grep "total"
Tool results:
→ result (ok): const [{ rows, total, isLoading }, setDocumentsState] = useState<{ - total: number | undefined + total: number | undefined; total: undefined, - const { data, total: fetchedTotal } = await searchDocuments(appliedFilters) - setDocumentsState({ rows: data, total: fetchedTotal, isLoading: false }) + const { data, total: fetchedTotal } = + total: fetchedTotal, + setDocumentsState({ rows: [], total: 0, isLoading: false });
This confirms it: the fix commit changed total: 0 to total: undefined in the catch block. That behavioral difference (whether the result count badge shows "0 results" or is hidden) is untested. Now I have the complete picture.
Summary
The three improvements address the specific feedback given and are mechanically correct. However, one has a structural flaw, and there is one notable behavioral gap introduced by this same fix commit that no test covers.
Critical Gaps
1. The non-Error fallback test asserts message === title outside waitFor — it is racy (criticality: 7/8)
At line 229-230 in the test file:
await vi.waitFor(() => {
expect(addToast).toHaveBeenCalledWith(...)
})
// Non-Error fallback: message falls back to the i18n title (not the thrown value)
const firstCall = vi.mocked(addToast).mock.calls[0]?.[0]
expect(firstCall?.message).toBe(firstCall?.title)
The waitFor block guarantees that addToast was called with objectContaining({ uiStyle: 'error', ... }). But the equality assertion on firstCall?.message and firstCall?.title runs immediately after, synchronously, outside the waitFor. While this happens to work because waitFor already resolved and mock.calls[0] is populated, the pattern is fragile: the title: expect.any(String) assertion inside waitFor does not constrain that title is non-empty, and the message: expect.any(String) assertion does not prevent a regression where message is the raw thrown string value "string error" rather than the i18n title. The key invariant ("thrown non-Error values are NOT exposed to users") could regress without this test failing if addToast happens to get called with message: 'string error' — because expect.any(String) still passes.
The test should instead assert message and title to the same concrete i18n string value, or at minimum use expect.not.stringContaining('string error') to guard the actual risk. As-is, if someone accidentally changes errorTitle to String(error) for non-Errors, the expect.any(String) check passes and the regression goes undetected. This is the test that matters most for the fallback behavior, and it does not adequately protect it.
2. The total: undefined behavior after error is untested — a behavioral regression introduced in this fix commit (criticality: 6)
The fix commit silently changed the catch block from setDocumentsState({ rows: [], total: 0, isLoading: false }) to setDocumentsState({ rows: [], total: undefined, isLoading: false }). These produce different UI: with total: 0 the result count area would display "0 results"; with total: undefined, the resultCount passed to NhrDocumentListHeader is undefined ?? data?.length which, since rows is [], evaluates to 0 via data?.length. However the resultCount prop to NhrDocumentList is undefined, so NhrDocumentListHeader receives resultCount = undefined ?? 0 = 0.
The existing "stop loading and clear rows" test only checks that no cell roles exist — it does not verify what the result count badge shows after an error (no "0 results" text vs. some text). If the intent was to hide the count entirely after an error, that intent is not captured in any test. If the intent was to show "0 results", the test should assert that. Either way, this is a silent, uncovered UI behavior change.
Important Improvements
3. The title: expect.any(String) assertion in the Error toast test is too weak (criticality: 5)
At lines 203-209, the title is only asserted to be some string. The actual value comes from t('tpf_national_health_records_ui.errors.search.title'), which resolves to "Unable to load DMP documents" (EN) or the French equivalent. A regression where title is set to "" or some unrelated string would not be caught. Since the i18n key is stable and the EN fixture is "Unable to load DMP documents", asserting title: expect.stringContaining('load') or the full string would provide meaningful protection with negligible brittleness cost.
Test Quality Issues
4. The "stop loading" test comment references "72 cells" (skeleton) but does not verify loading state disappears explicitly
The comment at line 240 says "skeleton renders 72 cells; after error isLoading=false so the body is empty". The queryAllByRole('cell') assertion inside waitFor does correctly wait for the skeleton to disappear (since skeleton rows render as cells and their absence confirms isLoading: false). This is sound. However the comment is slightly misleading — it would be clearer to say the absence of cells proves the skeleton is gone. This is a minor clarity issue, not a correctness problem.
Positive Observations
- Moving
queryAllByRole('cell')inside the samewaitForblock as the table presence check is the right call. It avoids a race where the table is present but the skeleton cells are still rendering, and correctly uses the skeleton cell count as a proxy for loading state. - The "toast per failed fetch" test (triggering two sequential errors via filter changes) is a valuable integration-level assertion that catches the class of bug where the error handler short-circuits after the first failure.
- The "no toast on success" negative test is well-placed and guards against a regression where the toast fires unconditionally.
- The
restoreMocks: true+ never-resolving promise approach for the defaultsearchDocumentsmock is well-documented and structurally sound.
Bottom Line
The three improvements are directionally correct but the non-Error fallback test (point 1) has a structural gap that defeats its primary purpose: if someone accidentally exposes the raw thrown value to users, expect.any(String) will not catch it. That specific assertion should be tightened to either a concrete expected string or at minimum a not.toBe('string error') guard. The total: undefined behavior change (point 2) is a silent gap worth a targeted assertion in the "stop loading" test.