Project: treatment-plan · Branch: main · Mode: · Model: claude-sonnet-4-6 Stats: 1 user messages, 15 tool calls, tools used: Bash, Read. Conversation Turn 1 — User Review PR #3019 in the doctolib/treatment-plan repository for test coverage quality and completeness. Do NOT post any comments on the PR — report your findings as text output only. PR context - Title: feat(FHIR-2194): pass document to dropdown action callbacks and add isAvailable filter - Package: @doctolib/tpf-national-health-records-ui - Change type: Frontend feature (TypeScript/React, Vitest + RTL) What changed (functional) 1. DropdownActionOption.onClick now receives the NhrDocument as argument 2. New isAvailable?: (doc: NhrDocument) => boolean predicate — when false, option is hidden from that row 3. New 'link-primary' added to uiStyle union 4. MoreActions, ActionButton, NhrDocumentListModal updated accordingly Test files updated - ActionButton.test.tsx — 3 new tests for isAvailable - MoreActions.test.tsx — 7 cases updated; link-primary style test added - NhrDocumentListModal.test.tsx — integration test verifying document passed to onClick - NhrDocumentListTableBody.test.tsx — assertions updated for document parameter How to get the diff gh pr diff 3019 --repo doctolib/treatment-plan The local repo is at /Users/USER/workspace/doctolib/treatment-plan — you can read test files directly. What to focus on - Are all paths through isAvailable tested? (undefined, returns true, returns false, mixed) - Is the onClick(doc) call verified with the correct document object? - Edge cases: empty option list, all options filtered out, async onClick handlers - Are the link-primary style tests complete? - Missing test scenarios for NhrDocumentListModal threading - Test quality: are assertions meaningful (not just "doesn't crash")? Report: (1) what's well-covered, (2) gaps and missing scenarios, (3) severity of each gap. Turn 1 — Assistant I'll start by fetching the PR diff and reading the relevant test files to do a thorough analysis. Tools used: - Bash: gh pr diff 3019 --repo doctolib/treatment-plan 2>&1 | head -500 Tools used: - Bash: find /Users/USER/workspace/doctolib/treatment-plan/packages -type f -name ".tsx" | xargs grep -l "NhrDocumentList\|MoreActions\|ActionButton\|DropdownActionOption" 2>/dev/null | head -20 Tool results: → result (ok): diff --git a/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentList.stories.tsx b/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentList.stories.tsx index ee5f3198b1..d4e7919d7d 100644 --- a/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentList.stories.tsx +++ b/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentList.stories.tsx @@ -157,7 +157,9 @@ const sharedBySampleR …(truncated, 20813 more chars) Tool results: → result (ok): /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentList.stories.tsx /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentList.test.tsx /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentList.tsx /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-r …(truncated, 2525 more chars) Tools used: - Bash: gh pr diff 3019 --repo doctolib/treatment-plan 2>&1 | tail -400 Tool results: → result (ok): + }) + + it('should pass the row to shouldBeVisible', () => { + const shouldBeVisible = vi.fn().mockReturnValue(true) + const action = createMoreActionsAction([ + { key: 'archive', label: 'Archive', isAvailable: shouldBeVisible, onClick: vi.fn() }, + ]) + const row = nhrDocumentFixtureBuilder() + + setup() + + expect(shouldBeVisible).toHaveBeenCalledWith(row) + }) }) diff --git a/packages/tpf-national-health-records-ui/src/comp …(truncated, 16710 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListTable/NhrDocumentListTableBody/ActionButton.tsx Tool results: → result (ok): 1 import type { NhrDocument } from '@doctolib/tpf-national-health-records' 2 3 import type { ActionConfig } from '../../../../types' 4 import { MoreActions } from './MoreActions' 5 import { SimpleAction } from './SimpleAction' 6 7 type ActionButtonProps = { 8 action: ActionConfig 9 row: NhrDocument 10 } 11 12 export const ActionButton = ({ action, row }: ActionButtonProps) => { 13 if (action.kind === 'dropdown') { 14 if (action.options.length === 0) return null 15 16 return { …(truncated, 1356 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 { 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 type { ActionConfig, ColumnConfig, FilterConfig, NhrPatientInfo, SearchDocumentsFn } from '../../types' 7 import { NhrDocumentList } from '../NhrDocumentList/NhrDocumentList' 8 9 export type NhrDocumentListModalProps = { 10 isOpen: boo …(truncated, 2809 more chars) I now have a complete picture. The local main branch is the pre-PR state. I've read both the old code and the full PR diff. Let me do a final check on the NhrDocumentListTableBody.test.tsx — specifically the PR diff version of it shows expect(onDownload).toHaveBeenCalledWith(doc) as the change, but the file on disk still has the old version. Let me also check the fixture builder to understand what nhrDocumentFixtureBuilder() produces: Tools used: - Bash: find /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui -name "nhrDocumentFixtureBuilder" 2>/dev/null Tool results: → result (ok): /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/dist/test-helpers/fixtures/nhrDocumentFixtureBuilder.d.ts /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/dist/test-helpers/fixtures/nhrDocumentFixtureBuilder.js.map /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/dist/test-helpers/fixtures/nhrDocumentFixtureBuilder.js /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national- …(truncated, 552 more chars) Tools used: - Bash: find /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui -name "createMoreActionsAction*" 2>/dev/null 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/dist/business/factories/actions/createMoreActionsAction.js.map /Users/USER/workspace/doctolib/treatment-plan/packages/tpf …(truncated, 384 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/business/factories/actions/createMoreActionsAction.ts Tool results: → result (ok): 1 import { EllipsisVerticalRegularIcon } from '@doctolib/design-system-icons/EllipsisVerticalRegularIcon' 2 3 import type { DropdownActionConfig, DropdownActionOption } from '../../../types' 4 5 export const createMoreActionsAction = (options: DropdownActionOption[]): DropdownActionConfig => ({ 6 kind: 'dropdown', 7 key: 'more', 8 tooltip: 'More actions', 9 iconComponent: EllipsisVerticalRegularIcon, 10 variant: 'transparent', 11 options, 12 }) 13 Tools used: - Read: /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/business/factories/actions/createMoreActionsAction.test.ts Tool results: → result (ok): 1 import { EllipsisVerticalRegularIcon } from '@doctolib/design-system-icons/EllipsisVerticalRegularIcon' 2 import { describe, expect, it, vi } from 'vitest' 3 4 import type { DropdownActionOption } from '../../../types' 5 import { createMoreActionsAction } from './createMoreActionsAction' 6 7 const mockOption: DropdownActionOption = { 8 key: 'download', 9 label: 'Download', 10 onClick: vi.fn(), 11 } 12 13 describe('createMoreActionsAction', () => { 14 it('returns ActionConfig with ke …(truncated, 1200 more chars) I now have a thorough understanding of the PR. Let me compile my findings. --- Summary The PR introduces three related behavioral changes to @doctolib/tpf-national-health-records-ui: passing the NhrDocument row to onClick callbacks, filtering options per-row via isAvailable, and adding 'link-primary' to the uiStyle union. The test coverage is generally solid and the approach is behavior-first. However, the analysis below reveals several concrete gaps — some of which are important enough to merit action before merge. --- What's Well Covered MoreActions.test.tsx (PR version): Every uiStyle branch is covered end-to-end for the onClick(doc) contract. The neutral (Dropdown.Item), link-danger (Dropdown.Button), and newly added link-primary branches each have their own click-through test verifying expect(onClick).toHaveBeenCalledWith(row). This is good because MoreActions.tsx has two distinct render paths inside DropdownOption (the if (option.uiStyle) branch vs. the fallthrough), and all three style variants exercise both paths. ActionButton.test.tsx (PR version): The isAvailable filter logic is tested cleanly at three points — isAvailable returning true, returning false for all options (empty result), and the argument passed to it being the correct row object. These directly target the filter expression !opt.isAvailable || opt.isAvailable(row) in ActionButton.tsx. NhrDocumentListModal.test.tsx (PR version): The new integration test verifies the full threading chain: searchDocuments returns a document, the dropdown is opened, an option is clicked, and onClick receives the exact same document object. This is the most important regression scenario for the feature and it's covered. NhrDocumentListTableBody.test.tsx (PR version): The onClick(doc) assertion is updated from toHaveBeenCalledOnce() to toHaveBeenCalledWith(doc) — a meaningful improvement that would catch the regression of passing the wrong document or no document. --- Critical Gaps Gap 1 — Mixed isAvailable scenario not tested (ActionButton layer) Criticality: 8/10 The ActionButton filtering logic action.options.filter((opt) => !opt.isAvailable || opt.isAvailable(row)) is only tested with a uniform result: all options visible (one test), or the single option invisible (one test). The practical use case is a list where some options have isAvailable returning false and others return true (or have no isAvailable predicate at all). There is no test for: given three options where option A has isAvailable returning false, option B has isAvailable returning true, and option C has no isAvailable, the rendered dropdown shows exactly options B and C. Without this test, a future change that accidentally applies the filter to the wrong subset or inverts the logic for the mixed case would pass all existing tests. The relevant source is the filter expression in /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListTable/NhrDocumentListTableBody/ActionButton.tsx (post-PR line: const visibleOptions = action.options.filter((opt) => !opt.isAvailable || opt.isAvailable(row))). Gap 2 — isAvailable filtering is tested at ActionButton but not confirmed to flow through to the rendered dropdown options (MoreActions layer) Criticality: 7/10 ActionButton passes { ...action, options: visibleOptions } down to MoreActions. The ActionButton tests confirm visibleOptions.length === 0 → nothing rendered, and that shouldBeVisible is called with the row. But no test opens the dropdown and asserts which option labels are visible vs. absent after filtering. The existing ActionButton tests checking isAvailable: false only assert container.toBeEmptyDOMElement() (which is for the all-filtered case); the partial filter case described in Gap 1 means we can't tell if hidden options are actually absent from the DOM. This failure mode: a regression that passes all filtered options to MoreActions regardless of the filter result would not be caught. Gap 3 — NhrDocumentListModal loading overlay with dropdown action and isAvailable predicate Criticality: 6/10 NhrDocumentListModal.tsx wraps each DropdownActionOption.onClick to set isActionLoading. It also passes isAvailable through unchanged (the PR spreads ...option before overriding onClick). There is no test confirming that isAvailable still functions correctly after the wrapping — specifically that a hidden option (one whose isAvailable returns false) is not shown in the modal's rendered table. The existing modal test for isAvailable behavior is entirely absent; only the onClick(doc) threading is verified at the modal level. Gap 4 — NhrDocumentListTableBody does not test row-specific option filtering (per-row isAvailable across multiple rows) Criticality: 6/10 The NhrDocumentListTableBody tests cover "dropdown trigger per row" and "option onClick per row," but the per-row nature of isAvailable is not exercised at this level. With two rows where isAvailable returns true for row 1 and false for row 2, the expectation is that row 1 shows the dropdown trigger and row 2 does not. This is the scenario most likely to reveal a regression where the row argument is captured incorrectly (closure over the wrong variable during filter). The filtering logic lives in ActionButton.tsx which is called once per row, so an integration test at the NhrDocumentListTableBody level would provide the highest confidence for this scenario. --- Important Improvements Gap 5 — link-primary style rendering is not separately verified in MoreActions Criticality: 5/10 The new link-primary test in MoreActions.test.tsx confirms onClick(row) is called but does not assert the visual rendering — specifically that a Dropdown.Button (rather than Dropdown.Item) is used for link-primary, and that it is accessible. For link-danger, the equivalent test asserts the button is in the document by role before clicking. The link-primary test does the same (it clicks the button, which implicitly confirms it renders), so this is a minor gap — but there is no assertion that the uiStyle prop is forwarded to the design system component. If link-primary accidentally falls through to the Dropdown.Item branch (because the if (option.uiStyle) check is true for any truthy uiStyle value), the visual treatment would be wrong but all tests would still pass. Gap 6 — NhrDocumentListTableBody onClick(doc) assertion uses toHaveBeenCalledOnce() without confirming the argument Criticality: 5/10 (already noted as partially addressed by the PR) Looking at the diff again carefully: the PR changes line 166 from expect(onDownload).toHaveBeenCalledOnce() to expect(onDownload).toHaveBeenCalledWith(doc). However, the local file on disk at /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListTable/NhrDocumentListTableBody/NhrDocumentListTableBody.test.tsx line 166 still reads expect(onDownload).toHaveBeenCalledOnce(). This means the PR diff correctly proposes the fix but the local branch doesn't have it applied yet. Assuming the PR branch does contain the change, this is addressed. --- Test Quality Issues Issue 1 — NhrDocumentListModal test for onClick(doc) uses vi.useRealTimers() inconsistently In NhrDocumentListModal.test.tsx, the beforeEach sets vi.useFakeTimers() but the new test (line 181 in the PR diff) calls vi.useRealTimers() at the top of the test body. This pattern is consistent with the other tests in that file that need async behavior (searchDocuments resolves), so it is intentional. However, there is no corresponding call to restore fake timers in afterEach for this specific test …(truncated, 1904 more chars)