Project: workspace-doctolib · Branch: HEAD · Mode: ` · **Model:**claude-sonnet-4-6`
Stats: 1 user messages, 10 tool calls, tools used: Read, Bash.
Conversation
Turn 1 — User
Review these files for UpFront architecture compliance:
- /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/components/NhrDocumentList/NhrDocumentCategoryFilter/NhrDocumentCategoryFilter.hooks.ts
Context: Custom multi-select combobox component for a filter panel. Uses: - oxygen-input-field CSS classes directly (InputText lacks a right-element slot) - dl-new-dropdown CSS class for dropdown visual parity with Select/BetaComboBox - Tailwind arbitrary variant selectors to pierce Checkbox/v2 internals - useCallback for stable event listener reference - Custom hook extracted to .hooks.ts for event listener lifecycle
Check for: separation of concerns, naming conventions, anti-patterns, component structure compliance with UpFront patterns.
Turn 1 — Assistant
I'll review both files systematically against the UpFront architecture rules.
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 { IconButton } from '@doctolib/design-system/IconButton/v2'
4 import type { Option } from '@doctolib/design-system/Option'
5 import { useCallback, useId, useRef, useState } from 'react'
6
7 import { useNhrDocumentCategoryClose } from './NhrDocumentCategoryFilter.hooks'
8
9 const normalize = (s: string) => s.normalize('NFD').re
…(truncated, 3978 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentCategoryFilter/NhrDocumentCategoryFilter.hooks.ts
Tool results:
→ result (ok): 1 import { type RefObject, useEffect } from 'react'
2
3 export const useNhrDocumentCategoryClose = (
4 containerRef: RefObject<HTMLDivElement | null>,
5 onClose: () => void,
6 ) => {
7 useEffect(() => {
8 const handlePointerDown = (e: PointerEvent) => {
9 if (containerRef.current && !containerRef.current.contains(e.target as Node)) {
10 onClose()
11 }
12 }
13
14 const handleKeyDown = (e: KeyboardEvent) => {
15 if (e.key === 'Escape') onClose()
16
…(truncated, 344 more chars)
Now let me inspect the surrounding package structure to evaluate placement, naming, and export conventions.
Tools used:
Bash:find /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList -type f | sort
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentCategoryCell/index.ts /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentCategoryCell/NhrDocumentCategoryCell.stories.tsx /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentCategoryCell/NhrDocumentCategoryCell.test.tsx …(truncated, 5729 more chars)
Tools used:
Bash:find /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src -type f | sort
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, 20248 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentCategoryFilter/index.ts
Tool results:
→ result (ok): 1 export * from './NhrDocumentCategoryFilter' 2
Tools used:
Read:/Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentStatusFilter/NhrDocumentStatusFilter.tsx
Tool results:
→ result (ok): 1 import { Checkbox } from '@doctolib/design-system/Checkbox/v2'
2 import { Heading } from '@doctolib/design-system/Heading'
3
4 import type { NhrDocumentStatusFilterProps } from '../../../types'
5
6 export const NhrDocumentStatusFilter = ({ label, statuses, value, onChange }: NhrDocumentStatusFilterProps) => (
7 <>
8 <Heading level='h5'>{label}</Heading>
9 <div className='flex flex-col'>
10 {statuses.map((status) => (
11 <Checkbox
12 key={status.filterKey}
13
…(truncated, 327 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentCategoryFilter/NhrDocumentCategoryFilter.test.tsx
Tool results:
→ result (ok): 1 import { setup } from '@doctolib/test-helpers'
2 import { screen } from '@testing-library/react'
3 import { afterEach, describe, expect, it, vi } from 'vitest'
4
5 import { NhrDocumentCategoryFilter } from './NhrDocumentCategoryFilter'
6
7 const sampleOptions = [
8 { value: 'A', label: 'Report A' },
9 { value: 'B', label: 'Report B' },
10 { value: 'C', label: 'Report C' },
11 ]
12
13 describe('NhrDocumentCategoryFilter', () => {
14 afterEach(() => vi.clearAllMocks())
15
16 const
…(truncated, 8852 more chars)
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
Now I have a complete picture. Let me do one final check on the NhrDocumentStatusFilter directory (no index.ts) and the __tests__ path to confirm the test placement anomaly.
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:ls /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/__tests__/
Tool results:
→ result (ok): index.test.ts
Here is the full review.
Architecture Review: NhrDocumentCategoryFilter
Files reviewed:
- /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/components/NhrDocumentList/NhrDocumentCategoryFilter/NhrDocumentCategoryFilter.hooks.ts
Critical Issues (must fix)
1. onClose is not stable — useEffect re-registers listeners on every render
In NhrDocumentCategoryFilter.hooks.ts, the dependency array is [containerRef, onClose]. The onClose prop is close, which is a useCallback with an empty dependency array, so it is stable across renders — that part is fine. However the hook signature accepts a generic onClose: () => void with no contract enforcing stability. Any future caller passing a non-memoized function will silently re-register both pointerdown and keydown listeners on every render cycle. The hook must either document that onClose must be referentially stable, or eliminate the dependency entirely by using a ref pattern:
export const useNhrDocumentCategoryClose = (
containerRef: RefObject<HTMLDivElement | null>,
onClose: () => void,
) => {
const onCloseRef = useRef(onClose)
useEffect(() => { onCloseRef.current = onClose })
useEffect(() => {
const handlePointerDown = (e: PointerEvent) => {
if (containerRef.current && !containerRef.current.contains(e.target as Node)) {
onCloseRef.current()
}
}
const handleKeyDown = (e: KeyboardEvent) => {
if (e.key === 'Escape') onCloseRef.current()
}
document.addEventListener('pointerdown', handlePointerDown)
document.addEventListener('keydown', handleKeyDown)
return () => {
document.removeEventListener('pointerdown', handlePointerDown)
document.removeEventListener('keydown', handleKeyDown)
}
}, [containerRef]) // stable dep only
}
This is the established pattern for event-listener hooks accepting callbacks.
2. normalize is an inline module-level function that belongs in a helpers file
normalize on line 9 of NhrDocumentCategoryFilter.tsx is a pure utility function. The UpFront pattern for local helpers is a ComponentName.helpers.ts sibling file. Placing business logic directly in the component module conflates responsibilities. Extract it to NhrDocumentCategoryFilter.helpers.ts and import it:
// NhrDocumentCategoryFilter.helpers.ts
export const normalize = (s: string) =>
s.normalize('NFD').replace(/\p{Diacritic}/gu, '').toLowerCase()
Warnings (should fix)
3. displayLabel derivation and filteredOptions derivation are inline render-body logic that should be memoized or extracted
The firstLabel, isPlaceholder, displayLabel, normalizedQuery, and filteredOptions computations run unconditionally on every render. filteredOptions iterates and normalizes the full options array every render. These are derived values from props; they should be wrapped in useMemo or extracted to the .helpers.ts file as pure functions that the component calls with memoized results. For a filter component that could receive dozens of document categories, this is a real performance concern during typing in the search field.
const displayLabel = useMemo(
() => computeDisplayLabel(options, values),
[options, values],
)
const filteredOptions = useMemo(
() => filterOptions(options, searchQuery),
[options, searchQuery],
)
4. handleClear and handleInputClick are not memoized
These handlers are recreated on every render and passed as props to IconButton and the native input. If the component tree grows, this will cause unnecessary re-renders of child components. They should use useCallback:
const handleClear = useCallback(() => {
onClear()
setIsOpen(false)
setSearchQuery('')
}, [onClear])
const handleInputClick = useCallback(() => {
if (!isOpen) {
setSearchQuery('')
setIsOpen(true)
}
}, [isOpen])
5. NhrDocumentStatusFilter directory is missing its index.ts re-exporter
Every component directory in the codebase that sits alongside sibling components has an index.ts (see NhrDocumentCategoryCell, NhrDocumentNameCell, NhrProcedureDateCell, etc.). NhrDocumentStatusFilter at /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentStatusFilter/ has no index.ts. This breaks the established pattern and forces consumers of that component to import from the file path directly rather than the directory. This is outside the two reviewed files but was discovered in the structural scan and belongs in this review.
6. src/__tests__/index.test.ts is in the wrong location
The file at /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/__tests__/index.test.ts is a test file placed in a dedicated __tests__ subdirectory. UpFront rules require test files to be colocated with their source, not collected into a separate test directory. The tests/ (or __tests__/) directory under src/ should only contain test support files (setup, utilities), not test files themselves. The file should move to colocate with src/index.ts.
Suggestions (consider improving)
7. Inline className string on the fieldset is too dense
Line 108 of NhrDocumentCategoryFilter.tsx:
className='dl-new-dropdown absolute top-full left-0 w-full mt-[0.4rem] flex flex-col border-0 p-0 [&_label]:!py-[1.2rem] [&_label]:!px-[1.6rem] [&_label>span:last-child]:![font-size:1.4rem]'
The Tailwind arbitrary variant selectors ([&_label]:...) that pierce Checkbox/v2 internals are a known workaround for the missing right-element slot in InputText, as stated in the context. Given that, the approach is justified. However, extracting this class string into a named constant or a CSS module selector would improve readability and make the intentional override explicit to future maintainers without requiring them to parse the full class list inline.
8. aria-haspopup='dialog' may not match screen reader expectations for a listbox-style dropdown
The element uses role='combobox' with aria-haspopup='dialog'. The ARIA spec for combobox expects aria-haspopup='listbox' or 'tree' for most combobox patterns. A fieldset of checkboxes is not semantically a dialog. This will cause screen readers to announce the popup as a dialog rather than a selection list. Consider aria-haspopup='listbox' and adjusting the fieldset role if accessibility completeness is required.
9. useCallback on close in the component is only necessary because useNhrDocumentCategoryClose lists it as a dependency
If the fix in issue 1 is applied (ref pattern in the hook), the useCallback wrapping close in NhrDocumentCategoryFilter.tsx can be dropped, simplifying the component. These two are linked — fix the hook first, then remove the useCallback in the component.
Summary
The hook extraction to .hooks.ts and the overall component structure are correct. The re-exporter pattern via index.ts is in place and properly named. Named exports are used throughout. The structural concerns are: the unstable onClose dependency in the hook, the missing .helpers.ts extraction for normalize, and the missing memoization for derived values and handlers. The NhrDocumentStatusFilter missing index.ts and the misplaced __tests__/index.test.ts are pre-existing issues surfaced by the structural scan.