Project: workspace-doctolib · Branch: HEAD · Mode:  · Model: claude-opus-4-7

Stats: 1 user messages, 4 tool calls, tools used: Bash, Read.

Conversation

Turn 1 — User

Review the latest commit on branch fhir-2219_implement-multi-select-category-filter in the treatment-plan repo at /Users/USER/workspace/doctolib/treatment-plan. This is the third review round — two previous rounds already fixed: missing index.ts, placeholder prop, raw × character, triggerLabel in factory, jsx-a11y no-static-element-interactions, consistent-return, chevron outside trigger button, span not a label element.

This latest commit (git log -1 --format="%H %s") addressed:
- Move chevron inside trigger button
- Replace <span> with <label htmlFor> using useId()
- Document-level Escape listener ([] deps, like the pointerdown listener)
- displayLabel guard for firstLabel === ''
- Factory imports through index.ts
- aria-haspopup='listbox' and role='group'

Files changed:
- packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentCategoryFilter/NhrDocumentCategoryFilter.tsx
- packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentCategoryFilter/NhrDocumentCategoryFilter.test.tsx
- packages/tpf-national-health-records-ui/src/business/factories/filters/createNhrDocumentCategoryFilterConfig.tsx
- packages/tpf-national-health-records-ui/src/business/factories/filters/createNhrDocumentCategoryFilterConfig.test.tsx

Do a git diff HEAD~1 HEAD on these files. Review for any remaining issues: correctness, accessibility, edge cases, test coverage, code quality. Focus on things not already addressed. Confidence threshold 80+. Report issues by file:line. Keep response under 400 words.

Turn 1 — Assistant

I'll review the latest commit on the specified branch.

Tools used:

- Bash: git log -1 --format="%H %s" 2>&1

Tool results:

  → result (ok): 05e694aa24f9a46e2b9dec90af940442150f1ae4 fix(fhir-2219): address accessibility and UX issues in NhrDocumentCategoryFilter

Tools used:

- Bash: git branch --show-current && git log -1 --format="%H %s%n%n%b"

Tool results:

  → result (ok): fhir-2219_implement-multi-select-category-filter
05e694aa24f9a46e2b9dec90af940442150f1ae4 fix(fhir-2219): address accessibility and UX issues in NhrDocumentCategoryFilter

- Move chevron inside trigger button so clicking it toggles the dropdown
- Replace <span> label with <label htmlFor> for proper HTML label association
- Move Escape key listener to document level so it works while checkboxes have focus
- Guard displayLabel against ', +N' output when no values match any option
- Import NhrDocum
…(truncated, 172 more chars)

Tools used:

- Bash: git diff HEAD~1 HEAD -- 'packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentCategoryFilter/NhrDocumentCategoryFilter.tsx' 'packages/tpf-national-health-records-ui/src/co
…(truncated, 321 more chars)

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 18a0464f3..e59d19c1c 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, 7619 more chars)

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 { XmarkLargeRegularIcon } from '@doctolib/design-system-icons/XmarkLargeRegularIcon'
3	import { Checkbox } from '@doctolib/design-system/Checkbox/v2'
4	import type { Option } from '@doctolib/design-system/Option'
5	import { useEffect, useId, useRef, useState } from 'react'
6	
7	type NhrDocumentCategoryFilterProps = {
8	  label: string
9	  values: string[]
10	  options: Option[]
11	  placehold
…(truncated, 2861 more chars)

Now I have full context. Let me review the diff against the project guidelines.

Reviewing: Latest commit 05e694a on branch fhir-2219_implement-multi-select-category-filter covering 4 files in packages/tpf-national-health-records-ui/.

Review Findings

Important (80-89)

1. aria-haspopup='listbox' is misleading - dropdown is not a listbox
- File: packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentCategoryFilter/NhrDocumentCategoryFilter.tsx:71,85
- Confidence: 85
- The button declares aria-haspopup='listbox', but the dropdown itself uses role='group' and contains Checkbox components — not role='listbox' with role='option' children. Per ARIA, when aria-haspopup='listbox' is set, the popup must actually be a listbox. For a multi-select group of independent checkboxes, the conventional pattern is aria-haspopup='true' (generic) or aria-haspopup='dialog', paired with role='group' (which the popup already uses). Either change aria-haspopup to match the popup role, or restructure the popup as a real listbox with aria-multiselectable='true'.
- Fix: change aria-haspopup='listbox' to aria-haspopup='true' so it matches the role='group' popup.

2. Trigger button is not associated with the popup via aria-controls
- File: NhrDocumentCategoryFilter.tsx:67-77,84-96
- Confidence: 80
- The trigger sets aria-expanded and aria-haspopup, but lacks aria-controls pointing at the popup's id. Screen readers cannot programmatically jump from the trigger to the controlled region. Add an id (via useId()) on the popup <div> and reference it with aria-controls on the button when isOpen is true. This is the standard disclosure pattern used together with aria-expanded.

Notes on changes that look correct

- Document-level keydown listener with [] deps mirrors the pointerdown pattern — correct, and the new dual Escape tests (trigger-focused + checkbox-focused) verify both paths.
- useId() + <label htmlFor> properly associates the visible label with the trigger; the test now uses getByLabelText('Category').
- displayLabel guard firstLabel === '' correctly avoids ", +N" output; both component-level and factory-level tests cover the unknown-values case via screen.queryByText(/, \+/).
- Factory now imports through index.ts, satisfying the re-exporter convention in .claude/rules/frontend/structure/component-patterns.md.
- Chevron is inside the trigger button with aria-hidden='true' — clicking it now toggles the dropdown, and assistive tech will not announce the decorative icon.

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.test.tsx
- /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/business/factories/filters/createNhrDocumentCategoryFilterConfig.tsx
- /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/business/factories/filters/createNhrDocumentCategoryFilterConfig.test.tsx

Two ARIA issues remain at confidence 80+ (both in NhrDocumentCategoryFilter.tsx); previously raised concerns are properly addressed.