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

Project: treatment-plan · Branch: main · Mode: ` · **Model:**claude-sonnet-4-6`

Stats: 1 user messages, 13 tool calls, tools used: Bash, Read.

Conversation

Turn 1 — User

Review PR #3019 in the doctolib/treatment-plan repository for UpFront architecture compliance and React/TypeScript patterns. 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 (located at packages/tpf-national-health-records-ui/)
• Change type: Frontend feature

What changed

• types.ts: DropdownActionOption.onClick changed from () => void to (doc: NhrDocument) => void | Promise<void>; added isAvailable?: (doc: NhrDocument) => boolean; added 'link-primary' to uiStyle union
• ActionButton.tsx: filters options by isAvailable, passes document to MoreActions
• MoreActions.tsx: receives document, injects into onClick at click time
• NhrDocumentListModal.tsx: threads document through loading-state wrapper

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 files directly.

What to focus on

• Separation of concerns: are UI components mixing business logic?
• Component API design: is the isAvailable predicate pattern the right abstraction?
• Type design: is NhrDocument being passed at the right layer?
• UpFront package conventions (naming, exports, barrel files)
• Any anti-patterns specific to this codebase

Report findings categorized by severity (blocking / suggestion / nit).

Turn 1 — Assistant

I'll start by fetching the PR diff and reading the relevant source files.

Tools used:

• Bash: gh pr diff 3019 --repo doctolib/treatment-plan 2>/dev/null || echo "FAILED"

Tools used:

• Bash: find /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src -type f | sort

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

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/createMoreActionsAction.ts /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/bus …(truncated, 19810 more chars)

Now let me read the key files to understand the full picture.

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

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 can see the diff references the new version of the files but the local repo is on main (clean state) — so I'm reading the pre-PR state. Let me check the current state of the files that were modified to understand what's already there vs. what the PR is adding.

Tools used:

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

Tool results:

→ result (ok): 1 import { TableBody } from '@doctolib/design-system/TableBody' 2 import { TableCell } from '@doctolib/design-system/TableCell' 3 import { TableRow } from '@doctolib/design-system/TableRow' 4 import { TableSelectCell } from '@doctolib/design-system/TableSelectCell' 5 import type { NhrDocument } from '@doctolib/tpf-national-health-records' 6
7 import type { ActionConfig, ColumnConfig } from '../../../../types' 8 import { ActionButton } from './ActionButton' 9
10 export type NhrDocumentListTable …(truncated, 1105 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:

• Bash: find /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui -name "package.json" | head -3 | xargs -I{} cat {}

Tool results:

→ result (ok): { "name": "@doctolib/tpf-national-health-records-ui", "version": "14.1.0", "repository": { "type": "git", "url": "git+https://github.com/doctolib/treatment-plan.git" }, "keywords": [ "upfront", "modular frontend", "smaller-scope" ], "author": "@doctolib/fire", "license": "Doctolib", "doctolib": { "upfront-type": "smaller-scope" }, "type": "module", "sideEffects": [ "./src/i18n.ts" ], "scripts": { "dev": "vite --host -d", "dev:mfr": …(truncated, 8746 more chars)

The local repo is on main (pre-PR state). I have the full diff and the pre-change source files. Let me now read the src/__tests__ directory to check whether tests are properly colocated, and also look at the utils/ directory to understand the structure.

Tools used:

• Read: /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/__tests__/index.test.ts

Tool results:

→ result (ok): 1 import { describe, expect, it } from 'vitest' 2
3 describe('tpf-national-health-records-ui', () => { 4 it('should have basic test coverage', () => { 5 expect(true).toBe(true) 6 }) 7 }) 8

Tools used:

• Read: /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/index.ts

Tool results:

→ result (ok): 1 export { NhrDocumentList } from './components/NhrDocumentList/NhrDocumentList' 2 export { NhrDocumentListFilterPanel } from './components/NhrDocumentListFilterPanel/NhrDocumentListFilterPanel' 3 export type { 4 ActionConfig, 5 DropdownActionOption, 6 BasicActionConfig, 7 DropdownActionConfig, 8 SimpleActionConfig, 9 ActionsFactory, 10 ColumnConfig, 11 FilterConfig, 12 FilterRenderContext, 13 NhrPatientInfo, 14 NhrVisibilityEntry, 15 StatusOption, 16 NhrDocumentStatusF …(truncated, 2233 more chars)

Now I have a thorough picture. Let me check one more thing — whether isAvailable is exported from src/index.ts (since DropdownActionOption is exported) and whether there's a shouldBeVisible naming inconsistency in tests.

Tools used:

• Bash: grep -n "isAvailable\|shouldBeVisible" /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListTable/NhrDocumentLi …(truncated, 56 more chars)

Tool results:

→ result (ok): (Bash completed with no output)

Tools used:

• Bash: grep -rn "isAvailable\|shouldBeVisible\|link-primary" /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/ 2>/dev/null | grep -v ".test." | grep -v "stories"

Tool results:

→ result (ok): (Bash completed with no output)

The local files are still in the pre-PR state (the diff hasn't been applied). I have all the information I need from the diff. Let me now produce the review.

Architecture Review: PR #3019

Package: @doctolib/tpf-national-health-records-ui Scope of change: src/types.ts, MoreActions.tsx, ActionButton.tsx, NhrDocumentListModal.tsx, associated tests and stories.

Critical Issues (must fix)

1. NhrDocumentListModal.tsx wraps onClick with a loading guard but still hardcodes async (doc: NhrDocument) =>

File: /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.tsx (line 60)

The wrappedActions memo transforms each dropdown option by replacing its onClick with a new arrow function. In the new signature the wrapper correctly receives doc and threads it through. However, the inline annotation async (doc: NhrDocument) => introduces a direct import of NhrDocument into NhrDocumentListModal.tsx. This type is already leaked into MoreActions.tsx and ActionButton.tsx as part of this PR. That is architecturally sound for internal presentational components, but NhrDocumentListModal is the public-facing container. Typing the wrapper argument explicitly here couples the container to the domain type in a way that could be avoided:

// current (introduces explicit type annotation inside wrappedActions)
onClick: async (doc: NhrDocument) => {

// preferred — let TypeScript infer from the `option.onClick` signature
onClick: async (doc) => {

This is a type-annotation hygiene issue. The NhrDocument import already exists in NhrDocumentListModal.tsx (used for the rows state), so there is no new import required — but removing the explicit annotation is cleaner. Not a blocker on its own, but worth noting.

Severity: Warning (see below — combined with issue 2 this becomes a design concern).

2. isAvailable filtering is done in ActionButton (rendering layer), not in the business layer

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

const visibleOptions = action.options.filter((opt) => !opt.isAvailable || opt.isAvailable(row))

The UpFront rule is that src/business/ handles pure functions and domain logic, while components are presentational. The isAvailable predicate is a domain rule ("should this action be shown for this document?") being evaluated inline inside a component. This is a mild violation of the business/presentation boundary.

The correct home for this filtering is a helper in src/business/ or at minimum a .helpers.ts file colocated with ActionButton. The existing createMoreActionsAction factory in src/business/factories/actions/ is the natural extension point: callers could pass isAvailable predicates and the factory (or a thin filterActionOptions(action, doc) helper) would return a filtered DropdownActionConfig. The component would then receive an already-filtered config and remain purely structural.

This is not a blocker if the team has consciously decided to keep filtering in the component for simplicity, but it should be called out explicitly as a deviation from the pattern established by src/business/.

Severity: Warning

3. ActionButton.tsx (pre-PR state) does not pass row to MoreActions — the PR fixes this but introduces a shape mismatch

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

The pre-PR ActionButton reads:

return <MoreActions action={action} />

It never passed row to MoreActions, which is correct in the old design where onClick: () => void. After this PR, ActionButton passes doc={row} and also reconstructs a new DropdownActionConfig object via spread:

return <MoreActions action={{ ...action, options: visibleOptions }} doc={row} />

The spread creates a new object on every render. For a pure component like MoreActions this causes unnecessary re-renders if MoreActions ever receives React.memo. This is a minor performance nit but worth noting. The correct pattern is:

// pre-filter happens once, stable reference via useMemo or passed directly
const visibleOptions = useMemo(
  () => action.options.filter((opt) => !opt.isAvailable || opt.isAvailable(row)),
  [action.options, row]
)
if (visibleOptions.length === 0) return null
return <MoreActions action={action} visibleOptions={visibleOptions} doc={row} />

Or simply accept the spread as-is if MoreActions is never memoized (currently it is not). Flag as a nit rather than a blocker.

Warnings (should fix)

4. Naming inconsistency: isAvailable in production code vs. shouldBeVisible in tests

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

Two tests use the variable name shouldBeVisible for the mock:

it('should render the option when shouldBeVisible returns true for the row', () => {
  ...
  isAvailable: vi.fn().mockReturnValue(true),

it('should pass the row to shouldBeVisible', () => {
  const shouldBeVisible = vi.fn().mockReturnValue(true)
  ...
  isAvailable: shouldBeVisible,

The test descriptions and local variable names say shouldBeVisible; the property in the type and the production code says isAvailable. These are inconsistent. If the team considered shouldBeVisible as an alternative name during design and chose isAvailable, the tests should be updated to match. If shouldBeVisible was the intended name, types.ts should be updated. As shipped, this leaves the next developer uncertain about intent.

Fix: align test description text and local variable names to isAvailable.

5. DropdownActionOption.uiStyle now accepts 'link-primary' but Dropdown.Button handling does not differentiate it from 'link-danger'

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

The pre-PR code checks if (option.uiStyle) and renders a Dropdown.Button. The post-PR code adds 'link-primary' to the union but the conditional remains if (option.uiStyle) — any truthy value goes through Dropdown.Button. The existing FIXME comment (Dropdown.Button types are incorrect) implies this branch is already a type cast workaround. Adding another valid value to the union without verifying that Dropdown.Button actually accepts 'link-primary' at runtime is a risk. There is a test for it (should call option.onClick with the row when a primary option is clicked) that only asserts the click callback is invoked — it does not assert the button is actually rendered with primary styling.

Recommendation: Add a Storybook story or assertion that visually verifies link-primary renders as intended, and verify the Dropdown.Button design system component accepts this variant.

6. src/__tests__/index.test.ts is a tests/ directory anti-pattern

File: /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/__tests__/index.test.ts

This is a pre-existing issue, not introduced by this PR, but worth flagging because the PR adds tests and the overall package structure is under review. Per the UpFront rule, test files must be colocated with source. The src/__tests__/ directory is not a valid location for test files — it should be used only for test support (setup, utilities). The index.test.ts file there is a trivial placeholder (expect(true).toBe(true)) and sh …(truncated, 2507 more chars)