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.