Project: doctolib-doctolib · Branch: .invalid · Mode: ` · **Model:**claude-sonnet-4-6`
Stats: 1 user messages, 15 tool calls, tools used: Bash, Read.
Conversation
Turn 1 — User
Analyze test coverage for PR #3001 in the treatment-plan repo (branch fhir-2214_nhr-error-type-extension-and-modal-toast). The git working directory is /Users/USER/workspace/doctolib/treatment-plan.
Run git diff main...fhir-2214_nhr-error-type-extension-and-modal-toast to see all changes.
Context: This PR adds error handling to NhrDocumentListModal — wrapping fetchDocuments in try/catch that calls addToast on failure. 5 new tests were added in NhrDocumentListModal.test.tsx.
Key behavioral contract to verify is tested:
1. searchDocuments throwing Error → toast dispatched with error.message, uiStyle: "error"
2. searchDocuments throwing non-Error → toast dispatched with i18n title as fallback message
3. On error: loading stops, rows cleared
4. On filter change that errors → toast dispatched again
5. On success → no toast
Analyze test coverage quality and completeness. Are there missing edge cases?
Turn 1 — Assistant
I'll analyze the test coverage for this PR by examining the changes and tests.
Tools used:
Bash:cd /Users/USER/workspace/doctolib/treatment-plan && git diff main...fhir-2214_nhr-error-type-extension-and-modal-toast --name-only
Tool results:
→ result (ok): packages/tpf-national-health-records-ui/src/business/toast.ts packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.test.tsx packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.tsx packages/tpf-national-health-records-ui/src/locales/en/tpf_national_health_records_ui.json packages/tpf-national-health-records-ui/src/locales/fr/tpf_national_health_records_ui.json packages/tpf-national-health-records/src/business …(truncated, 13 more chars)
Tools used:
Bash:cd /Users/USER/workspace/doctolib/treatment-plan && git diff main...fhir-2214_nhr-error-type-extension-and-modal-toast
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..15715cdee --- /dev/null +++ b/packages/tpf-national-health-records-ui/src/business/toast.ts @@ -0,0 +1,11 @@ +import { GLOBAL_PUBSUB_REGISTRY } from "@doctolib/upfront-core"; + +type ToastProps = { + message: string; + title: string; + uiStyle: "info" | "success" | "error" | "warning"; +}; + +export function addToast(p …(truncated, 28512 more chars)
Now let me read the full test file and the component implementation 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 { 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 {
8 ActionConfig,
9 ColumnConfig,
10 FilterConfig,
11 NhrPatientInfo,
12 SearchDocumentsFn,
13 } from "../../types";
14 import { NhrDocumentList } from "../NhrDocumentL
…(truncated, 2961 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 i
…(truncated, 12239 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
3 type ToastProps = {
4 message: string;
5 title: string;
6 uiStyle: "info" | "success" | "error" | "warning";
7 };
8
9 export function addToast(props: ToastProps) {
10 GLOBAL_PUBSUB_REGISTRY.dispatch({ type: "toasts:add", payload: props });
11 }
12
Now let me look at the toast.ts unit situation and the NhrDocumentSearchResult type change to understand the full scope of the PR.
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:find /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src -name "*.test.*" | sort
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/tests/index.test.ts /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/createViewDocumentAction.test.ts /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/s …(truncated, 6288 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records/src/business/types/api.ts(1–50)
Tool results:
→ result (ok): 1 import type {
2 DateRange,
3 NhrDocument,
4 NhrDocumentFile,
5 } from "#business/types/document";
6 import type { NhrPatientInformation } from "#business/types/participant";
7
8 export type NhrResultStatus =
9 | "success"
10 | "skipped"
11 | "error"
12 | "partial success";
13
14 type NhrResult = {
15 status: NhrResultStatus;
16 message?: string;
17 };
18
19 // Document Search
20 export type NhrDocumentSearchParams = {
21 formats?: string[];
22 categories?: string[];
23
…(truncated, 659 more chars)
Tools used:
Bash:grep -r "NhrDocumentSearchResult\|addToast\|toast" /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src --include="*.ts" --include="*.tsx" -l 2>/dev/null | gr …(truncated, 25 more chars)
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/business/toast.ts /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.test.tsx /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.tsx
Tools used:
Bash:grep -r "SearchDocumentsFn\|SearchResult" /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/types --include="*.ts" --include="*.tsx" 2>/dev/null | head -40
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/src/types -type f | sort && find /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-nationa …(truncated, 71 more chars)
Tool results:
→ result (ok): bfs: error: /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/types: No such file or directory. /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/types.ts /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/index.ts /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/test-helpers/index.ts /Users/USER/workspace/doctolib/treatment-plan/packages …(truncated, 928 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/treatment-plan/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 } 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. Returns a string for lo
…(truncated, 2274 more chars)
Now I have everything I need for a complete analysis. Let me also verify whether the NhrDocumentSearchResult.error field is consumed anywhere in the UI package, since it was added to the api.ts type in this PR.
Tools used:
Bash:grep -rn "NhrDocumentSearchResult\|\.error\b" /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src --include="*.ts" --include="*.tsx" 2>/dev/null | grep -v ". …(truncated, 29 more chars)
Tool results:
→ result (ok): (Bash completed with no output)
Tools used:
Bash:grep -rn "NhrDocumentSearchResult\|\.error\b" /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records/src --include="*.ts" --include="*.tsx" 2>/dev/null | grep -v node_ …(truncated, 18 more chars)
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records/src/index.ts:8: NhrDocumentSearchResult, /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records/src/business/types/api.ts:36:export type NhrDocumentSearchResult = { /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records/src/business/types/index.ts:13: NhrDocumentSearchResult, /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records …(truncated, 850 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/treatment-plan/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. Here is the full analysis:
Summary
The 5 new tests adequately cover the primary behavioral contracts of the try/catch block. The critical paths (Error instance, non-Error throw, loading state reset, repeat errors on filter change, clean success path) are all present. The overall test quality is good: tests use vi.waitFor correctly for async assertions, mocking is module-level rather than argument-snooping, and the filter-change test constructs a realistic interaction sequence. There are no tests that are overfit to implementation detail.
That said, there are several meaningful gaps — one of which is a real behavioral bug that is currently untested, and a few others that test the exact contract specified in the code.
Critical Gaps
1. The title field of the dispatched toast is never asserted — the non-Error fallback message assertion is incomplete (Criticality: 8)
The core production contract for the non-Error branch is:
// component source
message: error instanceof Error ? error.message : errorTitle,
The test for the non-Error case ("should use the i18n title as message fallback") only asserts uiStyle: "error". It does not check that message equals the i18n title string. This means a future refactor that sets message: "" or message: "unknown error" would pass the test but silently break the UX promise.
Likewise, neither error test asserts the title property of the addToast call. If someone accidentally swaps title and message in the object passed to addToast, both the displayed toast title and the toast body would be wrong, but neither test would catch it.
Missing assertion example that should be added:
// In the non-Error test:
expect(addToast).toHaveBeenCalledWith({
title: "Unable to load DMP documents", // the i18n value
message: "Unable to load DMP documents", // same string as fallback
uiStyle: "error",
});
// In the Error test, strengthen to also assert title:
expect(addToast).toHaveBeenCalledWith({
title: "Unable to load DMP documents",
message: "Technical software error: Operation timed out.",
uiStyle: "error",
});
What regression it prevents: title and message getting silently swapped, or the non-Error fallback drifting to a non-i18n string.
2. The total: 0 reset on error is not verified — the "stop loading and clear rows" test only checks the table element (Criticality: 7)
The component sets { rows: [], total: 0, isLoading: false } on error. The test for this path verifies only that the table renders (i.e. isLoading is false), not that rows is empty or that the result count is 0 / hidden. A regression where rows from a previous successful fetch are preserved after an error would be invisible to the current test.
Concrete failure scenario: If the catch block were changed to setDocumentsState((prev) => ({ ...prev, isLoading: false })) (keeping previous rows), the test would still pass.
Suggested assertion to add in the "stop loading and clear rows" test:
// After waiting for table:
expect(screen.queryByText(/results/)).not.toBeInTheDocument();
// Or, if there is a "no results" empty-state element, assert that.
3. The NhrDocumentSearchResult.error field added to the type in this PR is completely untested (Criticality: 6)
NhrDocumentSearchResult gained an optional error?: { message: string; code?: string } field. This is a structural extension to a public shared type used by DmpNhrService and its adapter interface. However:
- No test verifies that consumers handle a result where
erroris set but the call does not throw (a "soft error" pattern). - No test verifies that the component ignores
result.erroron a successful resolve — so if a future adapter returns{ documents: [...], error: { message: "partial failure" } }, the component currently swallows it silently, which may or may not be intentional.
The PR adds the field but provides no guidance through tests about whether callers should inspect it. If the intent is that adapters signal soft errors through this field (rather than throwing), the component's current implementation ignores it entirely, and there is no test capturing that as the expected behavior.
Important Improvements
4. No test for searchDocuments throwing after being stable then changing (stale-closure / unmount scenario) (Criticality: 5)
The useEffect depends on both searchDocuments and appliedFilters. If a parent passes a new searchDocuments prop that immediately rejects, there is no test confirming that the toast fires and state resets correctly in that scenario (as opposed to a rejection on the initial mount). The filter-change error test partially covers this for appliedFilters, but not for a searchDocuments prop change mid-lifecycle.
5. The toast.ts module has zero unit tests (Criticality: 4)
/packages/tpf-national-health-records-ui/src/business/toast.ts is a new module that wraps GLOBAL_PUBSUB_REGISTRY.dispatch. It is always mocked in the modal test, so its actual behavior — dispatching with type: "toasts:add" and the correct payload shape — is never exercised by any test. Should the pubsub event name or payload structure change, no test would fail.
A minimal unit test for toast.ts would verify:
import { GLOBAL_PUBSUB_REGISTRY } from "@doctolib/upfront-core";
// mock GLOBAL_PUBSUB_REGISTRY.dispatch
// call addToast({ message: "m", title: "t", uiStyle: "error" })
// assert dispatch called with { type: "toasts:add", payload: { message: "m", title: "t", uiStyle: "error" } }
This is low-hanging fruit given the module is trivial, and it protects against the event type string being typo'd.
Test Quality Issues
6. await import inside individual tests for a module-level mock is fragile (Criticality: 3)
Several error tests use:
const { addToast } = await import("../../business/toast");
inside the test body, after the module-level vi.mock(...) at the top of the file. This works under Vitest because vi.mock is hoisted and the dynamic import returns the already-mocked module, but the pattern is unusual and can cause confusion. The idiomatic alternative — importing the mock reference at the top of the file — is cleaner and makes the test intent obvious:
import * as toastModule from "../../business/toast";
// ...
const addToast = vi.mocked(toastModule.addToast);
This isn't a bug risk but can confuse future maintainers and occasionally causes issues if the Vitest hoisting semantics change.
Positive Observations
- The
beforeEach/afterEachfake-timer setup is correct and the comment explaining whysearchDocumentsis re-created per test is valuable documentation. - Using
vi.waitForfor async assertions rather than fixedawaitdelays is the right approach for async state-driven behavior. - The filter-change error test is well-constructed: it waits for the first toast before triggering the second fetch, avoiding a race condition in the assertion count.
expect.objectContainingis used appropriately in the Error-message test since the full shape of the toast object is what matters, not a strict reference.- The success-path negative test (
should not dispatch a toast when searchDocuments resolves successfully) correctly anchors on a DOM element (findByText("SOPK test results")) as the synchronization point before asserting the mock was not called. - The
NhrDocumentSearchResulttype extension (addingerror?) is additive and non-breaking for all existing callers.
Files relevant to this analysis:
/Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.tsx- `/Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumen …(truncated, 369 more chars)