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

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

Conversation

Turn 1 — User

Review the recently modified component for simplification opportunities. Do NOT make changes — only report findings.

File to review: /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentCategoryFilter/NhrDocumentCategoryFilter.tsx

Context: This is a custom multi-select combobox component for a React/TypeScript healthcare app. It uses Oxygen design system CSS classes and Tailwind for layout. The component is fully controlled (no internal selection state — only isOpen and searchQuery are internal state).

Look for:
- Redundant code, unnecessary variables, or logic that can be expressed more simply
- The isPlaceholder intermediate variable — is it needed?
- The handleInputClick function — could the logic be inlined?
- The two useEffects — can they be merged? Any dependency array issues?
- The e.stopPropagation() call in handleClear — is it still needed given input and iconWrapper are sibling DOM elements (not ancestor/descendant)?
- The placeholder ?? '' in the input — the placeholder is already optional, is this guard needed?
- The searchQuery.trim() check — is the trim necessary or is an empty string check sufficient?
- Any className string concatenation that could be cleaner

Turn 1 — Assistant

I'll review the component for simplification opportunities.

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 { useEffect, useId, useRef, useState } from 'react'
6	
7	type NhrDocumentCategoryFilterProps = {
8	  label: string
9	  values: string[]
10	  options: Option[]
11	  placeholder?: string
12	  onToggle
…(truncated, 4401 more chars)

Here are the simplification findings for /Users/USER/workspace/doctolib/treatment-plan/packages/tpf-national-health-records-ui/src/components/NhrDocumentList/NhrDocumentCategoryFilter/NhrDocumentCategoryFilter.tsx.

Findings

1. isPlaceholder intermediate variable (lines 56-62) — REMOVABLE

The variable is only used once (in the ternary that builds displayLabel) and adds an extra layer of indirection. The nested ternary is also flagged in your guidelines. Consolidate into a single, clearer derivation:

const selectedLabels = options.filter((o) => values.includes(o.value)).map((o) => o.label)
const displayLabel = selectedLabels.length === 0
  ? ''
  : selectedLabels.length === 1
    ? selectedLabels[0]
    : ${selectedLabels[0]}, +${selectedLabels.length - 1}

Or, even better — avoid the nested ternary per your project rules. Switch to an if/else block or extract to a small helper:

function buildDisplayLabel(selected: string[]): string {
  if (selected.length === 0) return ''
  if (selected.length === 1) return selected[0]
  return ${selected[0]}, +${selected.length - 1}
}

This also removes the awkward double-negative logic where firstLabel === '' masks a stale-value scenario.

2. handleInputClick (lines 75-80) — INLINE-WORTHY

Three lines, single call site, and the if (!isOpen) guard exists only to prevent re-clearing search while typing in the open input. Could be inlined as:

onClick={() => {
  if (isOpen) return
  setSearchQuery('')
  setIsOpen(true)
}}

Marginal benefit; keeping it as a named function is also fine for readability. Personal preference call — but the current name handleInputClick doesn't describe intent (openDropdown would be clearer if kept).

3. The two useEffects (lines 30-54) — MERGEABLE with caveats

They can be merged into one effect listening to both pointerdown (document) and keydown (container). However, they currently differ in scope:

- pointerdown: attached to document
- keydown: attached to containerRef.current

The keydown listener has a real bug: the dependency array is empty but el = containerRef.current is read at mount time only. If containerRef.current is null on first render (it shouldn't be for this layout, but defensive code shouldn't rely on that), the listener never attaches. More importantly, attaching keydown to the container only fires when focus is inside the container — fine for Escape on the input, but a document-level listener would be more consistent with pointerdown.

Recommended merge:

useEffect(() => {
  function handlePointerDown(e: PointerEvent) {
    if (containerRef.current && !containerRef.current.contains(e.target as Node)) {
      setIsOpen(false)
      setSearchQuery('')
    }
  }
  function handleKeyDown(e: KeyboardEvent) {
    if (e.key === 'Escape') {
      setIsOpen(false)
      setSearchQuery('')
    }
  }
  document.addEventListener('pointerdown', handlePointerDown)
  document.addEventListener('keydown', handleKeyDown)
  return () => {
    document.removeEventListener('pointerdown', handlePointerDown)
    document.removeEventListener('keydown', handleKeyDown)
  }
}, [])

Bonus: extract a function close() since both branches do the same setIsOpen(false); setSearchQuery('') pair (also done in handleClear).

4. e.stopPropagation() in handleClear (line 69) — LIKELY REMOVABLE

The clear button lives inside oxygen-input-field__iconWrapper, which is a sibling of the <input> element (both children of oxygen-input-field__inputWrapper). Clicks on the IconButton bubble up through iconWrapper -> inputWrapper -> outer div -> document. They never traverse through the input, so handleInputClick cannot fire from a clear-button click.

The stopPropagation would only matter if there were a click handler on oxygen-input-field__inputWrapper or higher that reopens the dropdown — there isn't. The pointerdown document listener checks containerRef.current.contains(target) which is true for the clear button (it's inside the container), so propagation reaching the document is also a no-op.

Safe to remove. If kept, add a comment explaining why.

5. placeholder ?? '' (line 97) — UNNECESSARY GUARD

React handles placeholder={undefined} exactly like an absent attribute. Just write placeholder={placeholder}. The ?? '' is a defensive habit from controlled-value contexts (where undefined triggers the controlled/uncontrolled warning) but it doesn't apply to placeholder.

6. searchQuery.trim() check (line 64) — TRIM IS LOAD-BEARING

The trim does matter, but not for the truthiness check — it's for the case where the user types only whitespace ("   "). Without trim, a whitespace query would filter against "   ".toLowerCase() and likely match nothing, hiding all options.

Two equivalent options:

// Option A: trim once and compute lowercase once
const normalizedQuery = searchQuery.trim().toLowerCase()
const filteredOptions = normalizedQuery
  ? options.filter((o) => o.label.toLowerCase().includes(normalizedQuery))
  : options

This also fixes a minor inefficiency: the current code calls searchQuery.toLowerCase() once per option per render.

7. className concatenation — CLEANER WITH ARRAYS or clsx

Line 95 is the worst offender:

className={oxygen-input-field__input ${isOpen ? 'cursor-text' : 'cursor-pointer'} ${values.length > 0 ? 'pr-[5.5rem]' : 'pr-16'}}

If clsx/classnames is available in this package (very common in Doctolib frontend), prefer:

className={clsx(
  'oxygen-input-field__input',
  isOpen ? 'cursor-text' : 'cursor-pointer',
  values.length > 0 ? 'pr-[5.5rem]' : 'pr-16',
)}

Same for line 115. Worth checking the package's existing dependencies before adopting — if the rest of the package uses template literals, stay consistent.

8. Additional observations

- values.length > 0 repeated 3 times (lines 95, 96 implicitly via displayLabel, 103). Extract once: const hasSelection = values.length > 0.
- Inline style={{ position: 'absolute', ... }} on the fieldset (line 123) mixes inline styles with Tailwind. The values top: 100%, left: 0, width: 100%, marginTop: 0.4rem map cleanly to Tailwind: absolute top-full left-0 w-full mt-1 (or mt-[0.4rem] for exact parity). Move into the className.
- displayLabel becomes empty string when selected option no longer exists in options (the firstLabel === '' branch). Worth a comment — it's a stale-data guard, not obvious to a future reader.
- onChange={(e) => setSearchQuery(e.target.value)} could use the function keyword per project rules, but for inline event handlers arrow functions are conventional in this codebase. Keep as-is.
- Naming: o in options.find((o) => ...) and options.filter((o) => ...) is terse. option is more idiomatic and the codebase already uses it inside the JSX map (line 126).

Priority ranking

| Priority | Item | Rationale |
|---|---|---|
| High | (3) Merge useEffects + fix Escape listener attachment | Real bug-shaped behavior; cleaner lifecycle |
| High | (1) Replace nested ternary in displayLabel | Violates project rule against nested ternaries |
| Medium | (4) Remove e.stopPropagation() | Dead code; siblings don't propagate to each other |
| Medium | (5) Remove placeholder ?? '' | Pure noise |
| Medium | (6) Hoist searchQuery.trim().toLowerCase() | Correctness + perf in one |
| Low | (7) clsx for className | Style consistency, depends on package convention |
| Low | (2) Inline handleInputClick | Subjective |
| Low | (8) hasSelection extraction, fieldset inline-style -> Tailwind | Polish |

No functional behavior changes from any of the above except (3), where the Escape handler becomes more robust.