Project: doctolib-doctolib · Branch: .invalid · Mode: · Model: claude-sonnet-4-6 Stats: 1 user messages, 27 tool calls, tools used: Bash, Read. Conversation Turn 1 — User Final test coverage review of the 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 main...HEAD -- packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.test.tsx to see the test changes. Context: PR adds error toast handling to NhrDocumentListModal. Tests cover: 1. Error toast dispatched with Error.message when searchDocuments throws an Error 2. i18n fallback used as message (not raw thrown value) when non-Error is thrown — now asserts expect.not.stringContaining('string error') AND message === title 3. Loading stops + cells cleared (verified inside waitFor) after error 4. Toast dispatched per failed fetch when filters change 5. No toast on success Previous test gaps that were fixed: - Added title: expect.any(String) assertion to Error case - Added expect.not.stringContaining('string error') to guard non-Error fallback - Moved empty-cells check inside waitFor to avoid racing the skeleton Are there any remaining critical coverage gaps? Turn 1 — Assistant Let me examine the current state of the tests and the implementation to give a thorough assessment. Tools used: - Bash: cd ~/workspace/doctolib/treatment-plan && git diff main...HEAD -- packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.test.tsx Tools used: - Bash: cd ~/workspace/doctolib/treatment-plan && git diff main...HEAD -- packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/ Tool results: → result (ok): 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 index d91977595..6940d797b 100644 --- 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 @@ -8,6 +8,8 @@ …(truncated, 4599 more chars) Tool results: → result (ok): 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 index d91977595..6940d797b 100644 --- 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 @@ -8,6 +8,8 @@ …(truncated, 6537 more chars) Now let me read the full implementation and the existing test file to understand the complete picture. 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) 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, 11328 more chars) Now let me look at the toast business module and the broader context to understand what contracts need verifying. 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 diff main...HEAD -- packages/tpf-national-health-records-ui/src/business/ 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 new file mode 100644 index 000000000..c44c729f8 --- /dev/null +++ b/packages/tpf-national-health-records-ui/src/business/toast.ts @@ -0,0 +1,13 @@ +import { GLOBAL_PUBSUB_REGISTRY } from '@doctolib/upfront-core' +import type { ReactNode } from 'react' + +type ToastProps = { + message: ReactNode + title: string + uiStyle: 'info' | 'success' | 'error' | 'war …(truncated, 188 more chars) 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 import type { ReactNode } from 'react' 3 4 type ToastProps = { 5 message: ReactNode 6 title: string 7 uiStyle: 'info' | 'success' | 'error' | 'warning' 8 action?: { message: string; onClick: () => void } 9 } 10 11 export function addToast(props: ToastProps) { 12 GLOBAL_PUBSUB_REGISTRY.dispatch({ type: 'toasts:add', payload: props }) 13 } 14 Tools used: - Bash: cd ~/workspace/doctolib/treatment-plan && git diff main...HEAD -- packages/tpf-national-health-records-ui/ --name-only | sort Tool results: → result (ok): } const fetchDocuments = async () => { expect(await screen.findByText('42 results')).toBeInTheDocument() setDocumentsState((prev) => ({ ...prev, isLoading: true })) void fetchDocuments() "entry_point.open": "Open NHR", "entry_point.open": "Ouvrir NHR", "filters.date_selection.custom_range.label": "Custom date range", "filters.date_selection.custom_range.label": "Plage de dates personnalisée", "filters.date_selection.last_access.label": "Depuis ma dernière conne …(truncated, 8758 more chars) Now let me check the locale files for the exact i18n key, and look at whether addToast is a direct call or wraps GLOBAL_PUBSUB_REGISTRY, plus whether there's any test for toast.ts itself. Tools used: - Bash: find /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src -name ".json" | 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: find /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src -name "toast" | sort Tool results: → result (ok): /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/business/toast.ts Tools used: - Read: /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/locales/en/tpf_national_health_records_ui.json Tool results: → result (ok): 1 { 2 "entry_point.open": "Open NHR", 3 "errors.search.title": "Unable to load DMP documents", 4 "filters.date_selection.custom_range.label": "Custom date range", 5 "filters.date_selection.last_access.label": "Since I last accessed this DMP", 6 "filters.date_selection.last_access.message": "Last accessed on:", 7 "translation": "Translation" 8 } 9 Tools used: - Bash: find /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui -name ".test." | sort Tool results: → result (ok): /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/node_modules/@module-federation/cli/dist/__tests__/readConfig.integration.test.d.ts /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/node_modules/@storybook/test/node_modules/@testing-library/jest-dom/types/__tests__/bun/bun-custom-expect-types.test.ts /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/node_modules/@storybook/test/node_modu …(truncated, 8102 more chars) Tools used: - Bash: grep -r "addToast\|toast" /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src --include=".test." -l 2>/dev/null Tool results: → result (ok): /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.test.tsx Tools used: - Bash: grep -r "isActionLoading\|action-loading-overlay\|wrappedActions\|finally" /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src --include=".test." 2>/dev/nu …(truncated, 13 more chars) Tool results: → result (ok): /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.test.tsx: expect(screen.getByTestId('action-loading-overlay')).toBeInTheDocument() /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.test.tsx: expect(screen.queryByTestId('action-loading-overlay')).not.toBeInTheDocument() Tools used: - Bash: grep -r "action.error\|onClick.reject\|action.throw\|action.catch" /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src --include=".test." 2>/dev/null | …(truncated, 9 more chars) Tool results: → result (ok): (Bash completed with no output) Tools used: - Bash: grep -n "isOpen.false\|modal.closed\|toast\|addToast" /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListMo …(truncated, 35 more chars) Tool results: → result (ok): 11:vi.mock('../../business/toast', () => ({ addToast: vi.fn() })) 49: it('should not be visible when isOpen is false', () => { 50: setup() 194: it('should dispatch an error toast with the error message when searchDocuments throws an Error', async () => { 196: const { addToast } = await import('../../business/toast') 202: expect(addToast).toHaveBeenCalledWith( 214: const { addToast } = await impor …(truncated, 713 more chars) Tools used: - Bash: grep -n "isOpen.false\|modal.closed\|unmount\|cleanup\|abort\|cancel\|unmounted" /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocument …(truncated, 46 more chars) Tool results: → result (ok): (Bash completed with no output) Tools used: - Bash: grep -n "useEffect\|cleanup\|return\|abort" /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.tsx 2>/d …(truncated, 7 more chars) Tool results: → result (ok): 5:import { useEffect, useMemo, useState } from 'react' 44: useEffect(() => { 86: return ( Tools used: - Bash: grep -n "isOpen" /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.tsx 2>/dev/null Tool results: → result (ok): 12: isOpen: boolean 23: isOpen, 87: Tools used: - Bash: grep -rn "clearAllMocks\|restoreMocks\|resetMocks" /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/ --include=".ts" --include=".json" --exclude-dir=node_mo …(truncated, 28 more chars) Tool results: → result (ok): (Bash completed with no output) Tools used: - Bash: find /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui -name "vitest.config" -o -name "vite.config" | grep -v node_modules | sort Tool results: → result (ok): /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/.storybook/vitest.config.ts /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/vite.config.ts /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/vitest.config.ts Tools used: - Read: /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/vitest.config.ts Tool results: → result (ok): 1 import { importSingleTs } from 'import-single-ts' 2 import { defineConfig, mergeConfig } from 'vitest/config' 3 import { sharedAliases } from '../../vitest-shared.config' 4 5 const upFrontVitest = (await importSingleTs('@doctolib/upfront-dev/vitest.config.ts')).default 6 7 export default defineConfig((configEnv) => 8 mergeConfig(upFrontVitest(configEnv), { 9 test: { 10 name: '@doctolib/tpf-national-health-records-ui', 11 root: new URL('./', import.meta.url).pathname, // …(truncated, 409 more chars) Tools used: - Bash: find /Users/USER/workspace/doctolib/treatment-plan -name "vitest.config.ts" -path "/upfront-dev/" | grep -v node_modules | head -3 Tools used: - Bash: find /Users/USER/workspace/doctolib/treatment-plan -path "*/upfront-dev/vitest.config.ts" 2>/dev/null | head -3 Tool results: → result (ok): (Bash completed with no output) Tool results: → result (ok): (Bash completed with no output) Tools used: - Bash: find /Users/USER/workspace/doctolib/treatment-plan/node_modules/@doctolib/upfront-dev -name "vitest.config.ts" 2>/dev/null | head -3 Tool results: → result (ok): /Users/USER/workspace/doctolib/treatment-plan/node_modules/@doctolib/upfront-dev/bases/vitest.config.ts /Users/USER/workspace/doctolib/treatment-plan/node_modules/@doctolib/upfront-dev/bases/storybook/vitest.config.ts Tools used: - Read: /Users/USER/workspace/doctolib/treatment-plan/node_modules/@doctolib/upfront-dev/bases/vitest.config.ts Tool results: → result (ok): 1 // A core Vite configuration for UpFront packages 2 // 3 // @see https://vitest.dev/config/ 4 import { configDefaults, defaultExclude, defineConfig } from 'vitest/config' 5 import { packageDirectory } from 'pkg-dir' 6 import react from '@vitejs/plugin-react' 7 import { resolve as resolvePaths } from 'node:path' 8 9 import { getEnvDefines } from './env/vite-defines' 10 11 // DEV NOTE: there is zero situation where packageDirectory() wouldn't return 12 // a string in our development patterns …(truncated, 2145 more chars) Now I have a complete picture. Let me check one final important detail — whether the useEffect has no cleanup and what that means for in-flight requests when isOpen toggles. Tools used: - Bash: grep -n "isOpen\|searchDocuments\|appliedFilters\|useEffect" /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentL …(truncated, 12 more chars) Tool results: → result (ok): 5:import { useEffect, useMemo, useState } from 'react' 12: isOpen: boolean 18: searchDocuments: SearchDocumentsFn 23: isOpen, 29: searchDocuments, 41: const [appliedFilters, setAppliedFilters] = useState>({}) 44: useEffect(() => { 49: const { data, total: fetchedTotal } = await searchDocuments(appliedFilters) 67: }, [searchDocuments, appliedFilters]) 87: 95: …(truncated, 32 more chars) I now have everything I need for a complete assessment. Here is the analysis. --- Summary The five new tests are well-scoped, test behavioral contracts rather than implementation details, and cover the primary happy/sad paths introduced by this PR. The mock isolation via vi.mock at module scope with restoreMocks: true in the vitest config is correctly handled. Overall coverage quality is good. There are two remaining gaps worth noting — one moderate and one minor — and one test quality concern. --- Critical Gaps None at the critical (8-10) level. --- Important Improvements 1. No toast when the component unmounts or closes while a fetch is in-flight (rating: 6/10) The useEffect at line 44-67 of NhrDocumentListModal.tsx has no cleanup function — it does not cancel the in-flight fetchDocuments promise. The useEffect dependency array is [searchDocuments, appliedFilters], and isOpen is deliberately excluded from it. This means: - If searchDocuments throws after the modal has been closed (the Modal hides but the component is still mounted), addToast is still called on unmounted or hidden state, and setDocumentsState is called on a component that the user has already dismissed. - There is no test asserting that closing the modal mid-fetch does not fire a spurious error toast when the promise eventually rejects. The scenario: open the modal, the fetch starts, close the modal before it resolves, the fetch then rejects — a toast fires even though the user dismissed the UI. This is a real UX regression that could appear in production under slow network conditions. What to test: render with isOpen={true}, capture the deferred promise, trigger close (onClose), then reject the promise, and assert addToast was NOT called (or that setDocumentsState did not update the state after unmount). The test would catch any future addition of a cleanup/AbortController that accidentally breaks this path, or confirm the current behavior is intentional. 2. The title field is never asserted to be the specific i18n string — only expect.any(String) (rating: 5/10) Both the Error case and the non-Error fallback case assert title: expect.any(String). If the t() call silently fails (missing key, misconfigured i18n in the test environment) and returns a key path like "tpf_national_health_records_ui.errors.search.title" or an empty string, the test still passes. The non-Error fallback test additionally asserts message === title, but neither checks the human-readable value. The locale file at /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/locales/en/tpf_national_health_records_ui.json has "errors.search.title": "Unable to load DMP documents". Asserting title: 'Unable to load DMP documents' in the Error case would catch i18n misconfiguration silently falling through. This is low-risk if the test environment is confirmed to use real i18n resolution, but the current setup file would need to be verified. --- Test Quality Issues The addToast import pattern creates a latent ordering risk Each error test does const { addToast } = await import('../../business/toast') inside the test body, after vi.mock has been declared at module scope. This works correctly today because vi.mock is hoisted and the dynamic import returns the mocked module. However, the vi.mocked(addToast) call at line 230 in the non-Error fallback test accesses .mock.calls[0]?.[0] with optional chaining and no null-assert, meaning if addToast was never actually called, firstCall would be undefined and the test would silently pass without verifying anything meaningful. Concretely: if a future refactor stops calling addToast in the non-Error branch, the waitFor assertion would catch it, but only because expect.objectContaining(...) requires the call to have happened. The trailing expect(firstCall?.message).toBe(firstCall?.title) would evaluate to expect(undefined).toBe(undefined) which is vacuously true. The guard is the waitFor block above — it would fail first — so this is not a real gap currently, but it is worth noting that the final two lines of the non-Error test provide weaker guarantees than they appear to. --- Positive Observations - Mocking ../../business/toast at the module level with vi.mock rather than spying on addToast directly is the correct approach; it prevents the real GLOBAL_PUBSUB_REGISTRY.dispatch from being called in tests. - Using vi.waitFor with the expectation inside (rather than await somePromise; expect(...) after) is the right pattern for async side-effects in vitest/RTL. - Moving the empty-cell assertion inside waitFor correctly avoids the skeleton-vs-settled race that was previously present. - The filter change test (test 4) correctly calls vi.clearAllMocks() implicitly via restoreMocks: true per-test, and uses toHaveBeenCalledTimes(2) rather than a weaker matcher, ensuring both fetches fire a toast. - The success test (test 5) anchors on a rendered document title (findByText('SOPK test results')) before asserting not.toHaveBeenCalled(), which correctly awaits the settled state before making the negative assertion.