Project: workspace-doctolib · Branch: HEAD · Mode: ` · **Model:**claude-sonnet-4-6`
Stats: 1 user messages, 3 tool calls, tools used: Read.
Conversation
Turn 1 — User
Final architecture review pass for NhrDocumentCategoryFilter. Previous pass findings that have been fixed: - normalize extracted to NhrDocumentCategoryFilter.helpers.ts - Hook uses ref-sync pattern (onCloseRef) — no more onClose in useEffect deps - Component no longer needs useCallback for close (plain function, ref pattern handles stability) - useMemo added for displayLabel and filteredOptions - aria-haspopup changed from 'dialog' to 'listbox'
Files to review: 1. /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentCategoryFilter/NhrDocumentCategoryFilter.tsx 2. /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentCategoryFilter/NhrDocumentCategoryFilter.hooks.ts 3. /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentCategoryFilter/NhrDocumentCategoryFilter.helpers.ts
Report ONLY new issues not already addressed. If the architecture looks clean now, say so explicitly.
Turn 1 — Assistant
I'll read all three files in parallel.
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 { useId, useMemo, useRef, useState } from 'react'
6
7 import { normalize } from './NhrDocumentCategoryFilter.helpers'
8 import { useNhrDocumentCategoryClose } from './NhrDocumentCategoryFilter.hook
…(truncated, 3927 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, useRef } from 'react'
2
3 export const useNhrDocumentCategoryClose = (
4 containerRef: RefObject<HTMLDivElement | null>,
5 onClose: () => void,
6 ) => {
7 const onCloseRef = useRef(onClose)
8 useEffect(() => {
9 onCloseRef.current = onClose
10 })
11
12 useEffect(() => {
13 const handlePointerDown = (e: PointerEvent) => {
14 if (containerRef.current && !containerRef.current.contains(e.target as Node)) {
15 onCloseRef.curren
…(truncated, 476 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentCategoryFilter/NhrDocumentCategoryFilter.helpers.ts
Tool results:
→ result (ok): 1 export const normalize = (s: string) => 2 s.normalize('NFD').replace(/\p{Diacritic}/gu, '').toLowerCase() 3
All three files are in hand. Here is the review of new issues only.
Architecture Review: NhrDocumentCategoryFilter (final pass)
Critical Issues (must fix)
None. The previously reported violations are resolved.
Warnings (should fix)
1. useNhrDocumentCategoryClose unconditionally attaches global listeners regardless of open state
/Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentCategoryFilter/NhrDocumentCategoryFilter.hooks.ts
The hook registers pointerdown and keydown on document at mount time and never removes them until unmount. When the dropdown is closed, those listeners fire on every pointer and key event unnecessarily. The hook has no awareness of isOpen.
The fix is to accept an isOpen: boolean parameter and gate the effect on it:
export const useNhrDocumentCategoryClose = (
containerRef: RefObject<HTMLDivElement | null>,
onClose: () => void,
isOpen: boolean,
) => {
const onCloseRef = useRef(onClose)
useEffect(() => {
onCloseRef.current = onClose
})
useEffect(() => {
if (!isOpen) return
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, isOpen])
}
Call site in the component becomes:
useNhrDocumentCategoryClose(containerRef, close, isOpen)
2. handleClear duplicates the close function body
/Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentCategoryFilter/NhrDocumentCategoryFilter.tsx, lines 51-55
close already sets isOpen(false) and clears searchQuery. handleClear repeats both state updates inline instead of calling close():
// current
const handleClear = () => {
onClear()
setIsOpen(false)
setSearchQuery('')
}
// correct
const handleClear = () => {
onClear()
close()
}
This is a maintainability issue: if close ever gains additional teardown (e.g. resetting focus), handleClear will silently diverge.
Suggestions (consider improving)
3. handleInputClick only opens, never toggles
/Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentCategoryFilter/NhrDocumentCategoryFilter.tsx, lines 57-61
The guard if (!isOpen) means clicking the trigger while open does nothing. The chevron icon visually suggests a toggle. Whether intentional (close is delegated to the outside-click handler) or not, a comment clarifying the design intent would prevent future developers from "fixing" the guard incorrectly.
4. onCloseRef sync effect has no dependency array
/Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentCategoryFilter/NhrDocumentCategoryFilter.hooks.ts, lines 8-10
useEffect(() => {
onCloseRef.current = onClose
})
Running on every render is the intentional ref-sync pattern and is correct. However, it is idiomatic to document this with a comment so the missing dependency array is not mistaken for a bug by a reviewer or a linter rule:
// Sync ref every render so the stable event listener always calls the latest onClose.
useEffect(() => {
onCloseRef.current = onClose
})
Summary
The architecture is clean. No separation-of-concerns violations, no naming convention violations, no misplaced files. The two warnings above are both in the component and hook files you listed and are straightforward to fix. The suggestions are cosmetic.