Project: workspace-doctolib · Branch: HEAD · Mode: ` · **Model:**claude-sonnet-4-6`
Stats: 1 user messages, 18 tool calls, tools used: Bash, Read.
Conversation
Turn 1 — User
Review the frontend architecture of changes in branch fhir-2219_implement-multi-select-category-filter of the treatment-plan repo at /Users/USER/workspace/doctolib/treatment-plan.
Get the diff with: git diff main...HEAD from that directory.
Changed files (all in packages/tpf-national-health-records-ui/src/):
1. components/NhrDocumentList/NhrDocumentCategoryFilter/NhrDocumentCategoryFilter.tsx — custom multi-select component
2. business/factories/filters/createNhrDocumentCategoryFilterConfig.tsx — factory with toggle/clear logic
Review for compliance with Doctolib's UpFront architecture patterns: - Separation of concerns (component vs factory) - Naming conventions - Component structure (fully controlled) - No anti-patterns
The relevant rules are in .claude/rules/frontend/ in the same repo.
Turn 1 — Assistant
I'll start by gathering the diff and reading the relevant architecture rules simultaneously.
Tools used:
Bash:git diff main...HEAD -- packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentCategoryFilter/NhrDocumentCategoryFilter.tsx packages/tpf-national-health-records-ui/src/busin …(truncated, 63 more chars)
Tool results:
→ result (ok): diff --git a/packages/tpf-national-health-records-ui/src/business/factories/filters/createNhrDocumentCategoryFilterConfig.tsx b/packages/tpf-national-health-records-ui/src/business/factories/filters/createNhrDocumentCategoryFilterConfig.tsx index d8e8982fa..6ccca21a3 100644 --- a/packages/tpf-national-health-records-ui/src/business/factories/filters/createNhrDocumentCategoryFilterConfig.tsx +++ b/packages/tpf-national-health-records-ui/src/business/factories/filters/createNhrDocumentCategoryFilt …(truncated, 5363 more chars)
Tools used:
Bash:ls /Users/USER/workspace/doctolib/treatment-plan/.claude/rules/frontend/
Tool results:
→ result (ok): cda-identifiers.md i18n.md ops react storybook.md structure
Tools used:
Bash:ls /Users/USER/workspace/doctolib/treatment-plan/.claude/rules/frontend/react/ && ls /Users/USER/workspace/doctolib/treatment-plan/.claude/rules/frontend/structure/
Tool results:
→ result (ok): oxygen-design-system.md react-hook-form.md react.md test component-patterns.md dependencies.md package-structure.md
Tools used:
Read:/Users/USER/workspace/doctolib/treatment-plan/.claude/rules/frontend/react/react.md
Tool results:
→ result (ok): 1 ---
2 paths:
3 - "packages//*.ts"
4 - "packages//.tsx"
5 - "packages//.css"
6 - "packages//*.scss"
7 ---
8
9 # Frontend React/TypeScript Conventions
10
11 ## Code Style
12
13 - ESLint + Prettier configuration via @doctolib/upfront-dev
14 - Pre-commit hooks**: Managed by Husky
15
16 ## Commands
17
18 - npm run lint - Run ESLint across all workspaces
19 - npm run lint:ci - Run ESLint for CI
20 - npm run test - Run tests across all workspaces
21 - `npm run for
…(truncated, 2304 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/treatment-plan/.claude/rules/frontend/structure/component-patterns.md
Tool results:
→ result (ok): 1 ---
2 paths:
3 - "packages//src/components//"
4 - "packages//.tsx"
5 ---
6
7 # Component Patterns
8
9 > For comprehensive UpFront guidelines, install the doctolib-frontend plugin (claude plugin add doctolib/prompts@main/plugins/doctolib-frontend). Key conventions below.
10
11 ## Component Directory Structure
12
13 Every public component has its own directory:
14
15 ```
16 file-viewer/
17 ├── file-viewer.tsx # The public component
18 ├── file-viewer.test.tsx # Te
…(truncated, 1463 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/treatment-plan/.claude/rules/frontend/structure/package-structure.md
Tool results:
→ result (ok): 1 ---
2 paths:
3 - "packages/*/"
4 ---
5
6 # Package Structure
7
8 > For comprehensive UpFront guidelines, install the doctolib-frontend plugin (claude plugin add doctolib/prompts@main/plugins/doctolib-frontend). Key conventions below.
9
10 ## Directory Layout
11
12 ```
13 package-name/
14 ├── src/
15 │ ├── assets/ # Static assets in original format
16 │ ├── business/ # Pure functions, domain types, shared business hooks
17 │ ├── components/ # React components (shar
…(truncated, 2397 more chars)
Now let me look at the full directory context and the existing structure around both changed files.
Tools used:
Bash:ls /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentCategoryFilter/
Tool results:
→ result (ok): NhrDocumentCategoryFilter.stories.tsx NhrDocumentCategoryFilter.test.tsx NhrDocumentCategoryFilter.tsx
Tools used:
Bash:ls /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/business/factories/filters/
Tool results:
→ result (ok): createNhrDateSelectionFilterConfig.test.tsx createNhrDateSelectionFilterConfig.tsx createNhrDocumentCategoryFilterConfig.test.tsx createNhrDocumentCategoryFilterConfig.tsx createNhrDocumentStatusFilterConfig.test.tsx createNhrDocumentStatusFilterConfig.tsx createNhrDocumentVisibilityFilterConfig.test.tsx createNhrDocumentVisibilityFilterConfig.tsx
Tools used:
Read:/Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentCategoryFilter/NhrDocumentCategoryFilter.tsx
Tool results:
→ result (ok): 1 import { ChevronDownRegularIcon } from '@doctolib/design-system-icons/ChevronDownRegularIcon'
2 import { Checkbox } from '@doctolib/design-system/Checkbox/v2'
3 import type { Option } from '@doctolib/design-system/Option'
4 import { useEffect, useRef, useState } from 'react'
5
6 type NhrDocumentCategoryFilterProps = {
7 label: string
8 values: string[]
9 triggerLabel: string
10 options: Option[]
11 placeholder?: string
12 onToggle: (option: Option) => void
13 onClear: () => void
…(truncated, 2158 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/business/factories/filters/createNhrDocumentCategoryFilterConfig.tsx
Tool results:
→ result (ok): 1 import type { Option } from '@doctolib/design-system/Option'
2
3 import { NhrDocumentCategoryFilter } from '../../../components/NhrDocumentList/NhrDocumentCategoryFilter/NhrDocumentCategoryFilter'
4 import type { FilterConfig } from '../../../types'
5
6 export const createNhrDocumentCategoryFilterConfig = ({
7 options = [],
8 label = 'Document Category',
9 placeholder,
10 }: {
11 options?: Option[]
12 label?: string
13 placeholder?: string
14 } = {}): FilterConfig => ({
15 key:
…(truncated, 1141 more chars)
Now let me check the index.ts for the component directory and look at other filter configs for comparison patterns.
Tools used:
Bash:ls /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentCategoryFilter/
Tool results:
→ result (ok): NhrDocumentCategoryFilter.stories.tsx NhrDocumentCategoryFilter.test.tsx NhrDocumentCategoryFilter.tsx
Tools used:
Read:/Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/business/factories/filters/createNhrDocumentStatusFilterConfig.tsx
Tool results:
→ result (ok): 1 import { NhrDocumentStatusFilter } from '../../../components/NhrDocumentList/NhrDocumentStatusFilter/NhrDocumentStatusFilter'
2 import type { FilterConfig, StatusOption } from '../../../types'
3
4 export const createNhrDocumentStatusFilterConfig = (statuses: StatusOption[], label: string): FilterConfig => ({
5 key: 'documentStatus',
6 render: (filters, onChange) => (
7 <NhrDocumentStatusFilter
8 label={label}
9 statuses={statuses}
10 value={statuses.filter((s) => fil
…(truncated, 158 more chars)
Tools used:
Bash:ls /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentStatusFilter/
Tool results:
→ result (ok): NhrDocumentStatusFilter.stories.tsx NhrDocumentStatusFilter.test.tsx NhrDocumentStatusFilter.tsx
Tools used:
Bash:git -C /Users/USER/workspace/doctolib/treatment-plan log --oneline fhir-2219_implement-multi-select-category-filter ^main | head -10
Tool results:
→ result (ok): 755f86d2d feat(fhir-2219): implement multi-select for Document Category filter
Tools used:
Bash:git -C /Users/USER/workspace/doctolib/treatment-plan diff main...HEAD -- packages/tpf-national-health-records-ui/src/
Tool results:
→ result (ok): diff --git a/packages/tpf-national-health-records-ui/src/business/factories/filters/createNhrDocumentCategoryFilterConfig.test.tsx b/packages/tpf-national-health-records-ui/src/business/factories/filters/createNhrDocumentCategoryFilterConfig.test.tsx index cabf73685..6b94c7168 100644 --- a/packages/tpf-national-health-records-ui/src/business/factories/filters/createNhrDocumentCategoryFilterConfig.test.tsx +++ b/packages/tpf-national-health-records-ui/src/business/factories/filters/createNhrDocum …(truncated, 16855 more chars)
I now have everything needed for a thorough review. Let me check the placeholder prop usage and the index.ts situation across sibling components for consistency.
Tools used:
Bash:ls /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/ | head -20
Tool results:
→ result (ok): NhrDocumentCategoryCell NhrDocumentCategoryFilter NhrDocumentList.stories.tsx NhrDocumentList.test.tsx NhrDocumentList.tsx NhrDocumentNameCell NhrDocumentStatusFilter NhrProcedureDateCell NhrShareDateCell NhrSharedByCell NhrStatusCell NhrVisibilityCell
Tools used:
Bash:find /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components -name "index.ts" | head -10
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrSharedByCell/index.ts /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrStatusCell/index.ts /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrVisibilityCell/index.ts /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health …(truncated, 511 more chars)
Tools used:
Bash:ls /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentStatusFilter/
Tool results:
→ result (ok): NhrDocumentStatusFilter.stories.tsx NhrDocumentStatusFilter.test.tsx NhrDocumentStatusFilter.tsx
I now have all the information needed. Here is the full review.
Architecture Review: fhir-2219_implement-multi-select-category-filter
Reviewed files:
- /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentCategoryFilter/NhrDocumentCategoryFilter.tsx
- /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/business/factories/filters/createNhrDocumentCategoryFilterConfig.tsx
Critical Issues (must fix)
1. Custom dropdown built from scratch instead of using an Oxygen Design System component
NhrDocumentCategoryFilter.tsx replaces BetaComboBox with a fully hand-rolled dropdown: raw <div>, <button>, manual mousedown listener for outside-click, manual Escape key handling, and hardcoded Tailwind utilities (border rounded, absolute z-10 w-full bg-white border shadow-md).
The architecture rule states: "Prefer design system components over custom HTML elements." A multi-select combobox/listbox is a well-solved accessibility problem. Building it from scratch introduces untested keyboard navigation (only Escape is handled — Tab, Arrow keys, and Home/End are absent), skips ARIA roles the design system would provide (role="listbox", role="option", aria-activedescendant), and diverges from the visual and behavioural contract the Oxygen token system defines.
Before accepting this component, verify whether Oxygen has a multi-select primitive — e.g., a MultiComboBox, a Select with multiple selection, or a Listbox — that covers this use case. If genuinely none exists, the hand-rolled implementation needs to be isolated behind an internal helper and must satisfy WCAG 2.1 AA for a combobox pattern before it is considered shippable.
2. triggerLabel computation belongs in the component, not the factory
createNhrDocumentCategoryFilterConfig.tsx computes triggerLabel inside the factory's render closure (lines 20–25 of the modified file) and passes it as a prop to the component:
const triggerLabel =
selectedValues.length === 0
? (placeholder ?? '')
: selectedValues.length === 1
? (selectedOptions[0]?.label ?? '')
: `${selectedOptions[0]?.label ?? ''}, +${selectedValues.length - 1}`
NhrDocumentCategoryFilter already receives values and options. From those two props it has all the information required to derive the display label internally. Computing presentation text in the factory couples the factory to the rendering detail of a specific UI variant — a direct violation of the Container/Presentational split. If the label format ever changes, the change must happen in the factory, not the component, which is the wrong locus of change.
Move triggerLabel derivation inside NhrDocumentCategoryFilter.tsx and remove the triggerLabel prop entirely. The factory should pass only the data props (values, options, label, placeholder) and the event callbacks.
Warnings (should fix)
3. Missing index.ts re-exporter
All cell-level components under NhrDocumentList (NhrDocumentCategoryCell, NhrDocumentNameCell, NhrStatusCell, NhrVisibilityCell, etc.) have an index.ts re-exporter. NhrDocumentCategoryFilter and NhrDocumentStatusFilter are the only two directories without one.
This branch adds no index.ts for NhrDocumentCategoryFilter/. Per the component-patterns rule, every public component directory must have index.ts exporting export * from './NhrDocumentCategoryFilter'. The absence pre-existed this branch for NhrDocumentStatusFilter, but the current change is the right moment to correct both — or at minimum the one being touched.
4. placeholder is accepted but never consumed
NhrDocumentCategoryFilterProps declares placeholder?: string and the prop is destructured in createNhrDocumentCategoryFilterConfig, but placeholder is passed down to NhrDocumentCategoryFilter (line 32 of the factory file) where it is silently ignored — the component's destructuring omits it. This is a dead prop on the component interface. Either remove it from the props type and the factory passthrough, or consume it (e.g., render it when triggerLabel is empty, which is what the factory already computes for the zero-selection case).
5. Factory imports component via deep relative path instead of path alias
createNhrDocumentCategoryFilterConfig.tsx line 3:
import { NhrDocumentCategoryFilter } from '../../../components/NhrDocumentList/NhrDocumentCategoryFilter/NhrDocumentCategoryFilter'
Three levels of ../ traversal qualifies for the #components/ alias per the package-structure rule ("Use # prefix for root path aliases" and "3+ ../ traversals"). This should be:
import { NhrDocumentCategoryFilter } from '#components/NhrDocumentList/NhrDocumentCategoryFilter'
(Once index.ts is added, the file-level segment at the end is also unnecessary.)
Suggestions (consider improving)
6. Outside-click uses mousedown instead of pointerdown
The useEffect listener registers mousedown. This misses touch events on mobile and stylus input. pointerdown is the unified equivalent and is already the convention used by Oxygen's own popover/dropdown internals. A low-risk swap.
7. isOpen state and the click-outside effect are generic UX hooks
useState(false) + useRef + document.addEventListener('mousedown', ...) is a classic "detect outside click" pattern. If any other component in the package ever needs a dropdown toggle, this logic will be duplicated. Per the hooks placement rules, a reusable useClickOutside hook belongs in src/hooks/. At this point the logic lives in a single component, so migration is optional — but the moment a second component needs it, it must be extracted.
8. The clear button uses a raw × character
<button ... aria-label='Clear'>×</button>
The visible content is a raw Unicode multiplication sign. Users relying on browser zoom or custom fonts may not render it reliably. Use ChevronDownRegularIcon is already imported from the icon system — an XmarkRegularIcon or equivalent from @doctolib/design-system-icons would be consistent with the rest of the design system and avoids the raw-character dependency.