Project: treatment-plan · Branch: main · Mode: · Model: claude-opus-4-7 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 code quality, style, and adherence to project guidelines. Do NOT post any comments on the PR — report your findings only as text output. 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) What changed - types.ts: DropdownActionOption.onClick changed from () => void to (doc: NhrDocument) => void | Promise; 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 - Test files and stories updated accordingly How to get the diff gh pr diff 3019 --repo doctolib/treatment-plan What to focus on - TypeScript correctness (no any, no eslint suppressions) - Naming conventions and consistency - Potential backward-compatibility issues with the onClick signature change - React patterns (hooks, props, memo, key usage) - Any dead code or TODOs left in - Adherence to project CLAUDE.md rules (no eslint-disable, no snapshot tests, etc.) Report findings categorized by severity (blocking / suggestion / nit). Turn 1 — Assistant I'll review PR #3019 by fetching the diff and examining the changes. Tools used: - Bash: gh pr diff 3019 --repo doctolib/treatment-plan 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) I have the full diff. Let me examine the key source files in more depth to understand the context and verify correctness. 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 { …(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) 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) Now let me check the createMoreActionsAction factory and examine the test descriptions for an issue I spotted. Tools used: - Bash: find /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui -type f -name "createMoreActionsAction" Tool results: → result (ok): /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-national-health-records-ui/dist/business/factories/actions/createMoreActionsAction.d.ts.map /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/dist/business/factories/actions/createMoreActionsAction.js /Users/USER/workspace/doctolib/treatment-plan/packages/ …(truncated, 384 more chars) Tools used: - Bash: grep -rn "isAvailable\|shouldBeVisible" /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui --include=".ts" --include=".tsx" Tool results: → result (ok): (Bash completed with no output) Tools used: - Bash: grep -rn "DropdownActionOption\|onClick" /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.tsx Tool results: → result (ok): /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.tsx:61: onClick: async () => { /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.tsx:65: await option.onClick() /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/ …(truncated, 266 more chars) Note: the grep output for the file on disk shows the pre-change* onClick signature for dropdown options. Let me confirm this by looking at the current file; since the diff is unstaged/unmerged, the working copy must represent the PR state. Let me check the current branch and file state. Tools used: - Bash: git log --oneline -5 main && git branch --show-current Tool results: → result (ok): 2b7ed9a59 chore(ATF-1657): remove MFR config from tpf-ldt-lab-integration (#2996) 1dd982255 feat(MOOSE-4134): add dev toolbar with scenario presets and Storybook integration [Stack 11/12] (#2958) cb850abc3 docs(SEAL-1733): strengthen Claude workflow routing in AGENTS.md (#3000) a995b1bdd feat(FHIR-2194)!: render dropdown for actions in DocList table (#2997) 0ef9d99fc feat(ACAI-4219): add GET /bundles/{bundle_id}/templates endpoint (#2972) main Tools used: - Bash: gh pr view 3019 --repo doctolib/treatment-plan --json headRefName,baseRefName,state,isDraft Tool results: → result (ok): {"baseRefName":"main","headRefName":"fhir-2194-3","isDraft":true,"state":"OPEN"} I'm on main. The working copy shows pre-PR state. My review is based on the diff from gh pr diff 3019 which I already have. Let me look at the createMoreActionsAction factory to ensure the PR works correctly. 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: gh pr view 3019 --repo doctolib/treatment-plan --json files --jq '.files[].path' Tool results: → result (ok): packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentList.stories.tsx 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/components/NhrDocumentListModal/NhrDocumentListTable/NhrDocumentListTableBody/ActionButton.test.tsx packages/tpf-national-health-records-ui/src/compon …(truncated, 737 more chars) Now I have a complete picture. Let me compile the review findings. PR Review: #3019 — feat(FHIR-2194): pass document to dropdown action callbacks and add isAvailable filter Files reviewed (all absolute paths): - /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/components/NhrDocumentListModal/NhrDocumentListTable/NhrDocumentListTableBody/ActionButton.tsx - /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListTable/NhrDocumentListTableBody/ActionButton.test.tsx - /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListTable/NhrDocumentListTableBody/MoreActions.tsx - /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListTable/NhrDocumentListTableBody/MoreActions.test.tsx - /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListTable/NhrDocumentListTableBody/NhrDocumentListTableBody.test.tsx - /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/NhrDocumentListModal/NhrDocumentListModal.test.tsx - /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/NhrDocumentListModal/NhrDocumentListTable/NhrDocumentListTableBody/NhrDocumentListTableBody.stories.tsx --- Blocking (Critical: 90–100) 1. Breaking change on a public API not marked with ! in the commit — confidence 92 File: /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/types.ts lines 80 & 82 - uiStyle widened from 'link-danger' to 'link-primary' | 'link-danger' (non-breaking, additive). - onClick signature changed from () => void | Promise to (doc: NhrDocument) => void | Promise (breaking — existing consumers whose callback has no parameter are still assignable via TS bivariance, but any consumer that relied on DropdownActionOption['onClick'] as a value-level () => void type will break at the type level, and any programmatic call-site that invoked it with no argument relies on doc being undefined — which is now a contract violation). Per the repo's CLAUDE.md (.claude/rules/general.md "Commit Message Format" section), breaking public-API changes MUST either append ! after the type/scope or include a BREAKING CHANGE: footer. The PR title is feat(FHIR-2194): pass document to dropdown action callbacks... — no !. The prior PR #2997 that introduced this component was correctly flagged feat(FHIR-2194)!:. This PR changes the same public surface of @doctolib/tpf-national-health-records-ui (the DropdownActionOption type is exported via types.ts) and must also be marked breaking. Fix: retitle commit / PR to feat(FHIR-2194)!: ... or add a BREAKING CHANGE: footer explaining the onClick signature change. --- Important (80–89) 2. NhrDocumentListModal wrappedActions wrapper is type-incorrect post-change — confidence 88 File: /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.tsx lines 58–71 After the diff: options: action.options.map((option) => ({ ...option, onClick: async (doc: NhrDocument) => { setIsActionLoading(true) try { await option.onClick(doc) } finally { setIsActionLoading(false) } }, })), The inline parameter annotation doc: NhrDocument duplicates a type that is now inferable from the DropdownActionOption.onClick signature. That is purely stylistic. But more importantly: option.onClick(doc) is passed through correctly; however the wrapper discards the isAvailable guard entirely — isAvailable is declared at the type level but the wrapper ...option spread only preserves it, and NhrDocumentList downstream does use it. The isActionLoading wrapper is fine. The real concern: the inline doc: NhrDocument annotation is redundant because the callback's parameter type is already inferred by the object literal being assigned to DropdownActionOption. Remove it for consistency (onClick: async (doc) => { ... }) — the surrounding code in the same file (line 76 for simple action) already uses the pattern async (doc: NhrDocument) => ..., so fine either way, but stay consistent. This is a style-only nit — downgraded from "important" to suggestion. See Suggestion #3 below. 3. Test description references a renamed/removed prop, confidence 85 File: /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListTable/NhrDocumentListTableBody/ActionButton.test.tsx lines 48 and 64 (added hunks) it('should render the option when shouldBeVisible returns true for the row', () => { ... }) it('should render nothing when shouldBeVisible returns false for all options', () => { ... }) it('should pass the row to shouldBeVisible', () => { ... }) The prop is named isAvailable (in types.ts and in ActionButton.tsx), not shouldBeVisible. Test names describe the behavior in terms of a prop name that does not exist in the source. The tests themselves set isAvailable: vi.fn()... correctly, so functionally they pass, but the it() descriptions are misleading. The third test even names the local variable const shouldBeVisible = vi.fn() and then passes it as isAvailable: shouldBeVisible — that is stale naming from an earlier draft. Fix: replace shouldBeVisible with isAvailable in all three test descriptions and in the local variable on the third test, to match the actual public API. --- Suggestions (Valid but lower impact: 70–79) 4. Grammar fix in test description is incomplete — confidence 75 File: /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListTable/NhrDocumentListTableBody/ActionButton.test.tsx lines 30, 39 The PR fixes "an DropdownActionConfig" → "a DropdownActionConfig" in two places, which is correct. No action needed — just noting it is good cleanup bundled with the feature. 5. MoreActions.tsx handleClick closure recreated on every render — confidence 70 File: /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListTable/NhrDocumentListTableBody/MoreActions.tsx line 15 const handleClick = () => option.onClick(doc) Acceptable since DropdownOption is a very small presentational leaf, but if the dropdown list becomes long this could be memoized with useCallback. Not a blocker; Oxygen Dropdown.Item / Dropdown.Button don't appear to be memoized consumers, so there is no measurable win. Skip unless perf profiling shows a problem. 6. NhrDocumentListModal.tsx inline NhrDocument type annotation is redundant — confidence 72 File: /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentListModal/NhrDocumentListModal.tsx line 61 The explicit doc: NhrDocument annotation is already inferable from DropdownActionOption['onClick']. Removing it (and keeping the import if still used elsewhere) shortens the closure and keeps the callback aligned with the type system: onClick: async (doc) …(truncated, 3836 more chars)