Session: review-the-pr-https-github-com-doctolib-shiny-axolotl-subagent-a243328f — 2026-04-23

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(<ActionButton action={action} row={row} />) + + 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 <Mo …(truncated, 102 more chars)

Now let me read the actual test files as they exist in the PR branch and the source files to understand the full picture:

Tools used:

• Read: /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListTable/NhrDocumentListTableBody/ActionButton.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 { describe, expect, it, vi } from 'vitest' 5
6 import { createMoreActionsAction } from '../../../../business/factories/actions/createMoreActionsAction' 7 import { nhrDocumentFixtureBuilder } from '../../../../test-helpers/fixtures/nhrDocumentFixtureBuilder' 8 import type { SimpleActionConfig } from '../../../ …(truncated, 1421 more chars)

Tools used:

• Read: /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListTable/NhrDocumentListTableBody/MoreActions.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 { describe, expect, it, vi } from 'vitest' 5
6 import { createMoreActionsAction } from '../../../../business/factories/actions/createMoreActionsAction' 7 import type { DropdownActionOption } from '../../../../types' 8 import { MoreActions } from './MoreActions' 9
10 const MockIcon = (() => <svg data …(truncated, 3243 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, 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, 8225 more chars)

I notice the local test files don't reflect the PR changes yet (the MoreActions.test.tsx still shows the old version without doc prop). Let me read the remaining test files and the source files to get the complete picture:

Tools used:

• Read: /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListTable/NhrDocumentListTableBody/NhrDocumentListTableBody.test.tsx

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 { setup } from '@doctolib/test-helpers' 3 import { screen, waitFor } from '@testing-library/react' 4 import { describe, expect, it, vi } from 'vitest' 5
6 import { createMoreActionsAction } from '../../../../business/factories/actions/createMoreActionsAction' 7 import { mockColumns, mockRows } from '../../../../test-helpers' 8 import { nhrDocumentFixtureBuilder } from '../../../../test-helpers/fixtures/nhr …(truncated, 5906 more chars)

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, 3393 more chars)

Now I have a clear picture. The local main branch has the old code (without the PR changes), and I've been reading the PR diff to understand exactly what the PR proposes. The local types.ts also shows the old API. Let me verify one more important piece — the NhrDocumentListTableBody.test.tsx in the PR diff vs what's on disk to confirm my understanding:

Tools used:

• Read: /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListTable/NhrDocumentListTableBody/MoreActions.tsx

Tool results:

→ result (ok): 1 import { Dropdown } from '@doctolib/design-system/Dropdown' 2 import { DropdownTrigger } from '@doctolib/design-system/DropdownTrigger' 3 import type { IconName } from '@doctolib/design-system/Icon' 4 import { IconButton } from '@doctolib/design-system/IconButton/v2' 5
6 import type { DropdownActionConfig, DropdownActionOption } from '../../../../types' 7
8 type DropdownOptionProps = { 9 option: DropdownActionOption 10 } 11
12 const DropdownOption = ({ option }: DropdownOptionProps) => {

…(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)