Project: clertonra-workspace · Branch: HEAD · Mode: plan · Model: claude-sonnet-4-6

Stats: 1 user messages, 2 tool calls, tools used: Agent, ToolSearch.

Conversation

Turn 1 — User

Review this PR https://github.com/doctolib/doctolib/pull/238392/. Take a look at packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.ts, which the returning function must run only on the component that call it. Analyse if the isOpen argument is needed, or if there is another viable alternative

Turn 1 — Assistant

Tools used:

- Agent (inputs: description, subagent_type, prompt)

Tool results:

  → result (ok): Excellent! Now I have all the information I need. Let me compile the final report.

Research Summary: PR #238392 - feat(FHIR-2088): Wire dmp documentSearch call

Full Content of useSearchDocuments.ts (Current - Before PR)

File path: /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.ts

import type { Patient } from 'monolith_DEPRECATED/types/doctor/api/Patient'
import { isFeatureEnabled } from '@doctolib/v

…(truncated, 5831 more chars)

Now I have a thorough picture of the code. Let me write the analysis plan and present my findings.

Tools used:

- ToolSearch (inputs: query, max_results)

Tool results:

  → result (ok)

This is a code review/analysis, not an implementation task — I don't need to produce a plan for approval. Here's my analysis:

---

★ Insight ─────────────────────────────────────
The hook returns a callback, not reactive data. useSearchDocuments returns an async function that only runs when the component explicitly calls it (e.g., on filter apply or mount effect). It does not auto-execute like useQuery. This distinction is critical to evaluating whether isOpen is necessary.
─────────────────────────────────────────────────

Analysis of isOpen in useSearchDocuments

What isOpen actually does in the PR

It adds an early-return guard inside the returned async function:

if (!isOpen) return { data: [] }

Its purpose (EX_3.1-2030): "no fetch until the modal is open."

It then gets prop-drilled through 3 layers:
component
  → useNhrDocumentListVariant(patient, isOpen)
    → useDmpDocumentListVariant(patient, isOpen)
      → useSearchDocuments({ ..., isOpen })

---

Is isOpen needed?

It depends on one critical question: is the consuming component conditionally mounted?

Case A — Component is conditionally rendered ({isOpen && <NhrDocumentList />})

isOpen inside the hook is redundant. When the modal is closed, the component doesn't exist, the hook doesn't run, and the returned function can never be called. React's own lifecycle makes the guard unnecessary.

Recommended alternative: remove isOpen from the hook entirely and rely on conditional mounting at the parent. This is the standard React pattern and eliminates the prop drilling.

Case B — Component is always mounted (e.g., kept alive for animations or layout reasons)

Then the guard matters — but isOpen being passed through 3 layers is still the least elegant solution. The idiomatic React Query approach is to keep the query disabled with the enabled flag:

// Inside useSearchDocuments, if using useQuery:
const { data } = useQuery({
  queryKey: [...],
  queryFn: fetchDocuments,
  enabled: isOpen,           // React Query's native "don't run yet" mechanism
  staleTime: 5  60  1000,
})

This isolates the concern at the query level, doesn't require threading isOpen through multiple hook layers, and is exactly what React Query's enabled was designed for.

---

Verdict

★ Insight ─────────────────────────────────────
Prop-drilling UI state into data hooks is an anti-pattern. Whether a modal is open is a UI concern; useSearchDocuments is a data concern. Mixing them couples the hook to a rendering decision it shouldn't know about.
─────────────────────────────────────────────────

| Approach | When to use | Verdict |
|---|---|---|
| Remove isOpen, use {isOpen && <Component />} | Component is conditionally mounted | Best — no coupling |
| React Query enabled: isOpen at query level | Component always mounted, uses useQuery internally | Good — idiomatic RQ |
| Current PR: prop-drill isOpen to hook | — | Avoid — leaks UI state into data layer |

My recommendation: Check whether the parent component that renders the document list already uses conditional rendering. If so, the isOpen argument and all its prop-drilling should be removed. If the component must stay mounted, refactor the internal fetch to use React Query's enabled: isOpen rather than threading the flag through the hook chain.