Project: doctolib-doctolib · Branch: .invalid · Mode:  · Model: claude-opus-4-7

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

Conversation

Turn 1 — User

You are reviewing PR #241005 in the doctolib/doctolib monorepo for code simplification opportunities. The goal is to identify unnecessarily complex, redundant, or over-engineered patterns in the new/changed code.

New files to review

useNhrDocumentStatusActions.ts
import { useCallback, useMemo, useState } from 'react'
import type { NhrDocument } from '@doctolib/tpf-national-health-records'
import type { DropdownActionOption } from '@doctolib/tpf-national-health-records-ui'
import type { Patient } from 'monolith_DEPRECATED/types/doctor/api/Patient'
import i18n from 'monolith_DEPRECATED/services/i18n'
import { isActive, isArchived } from '../variants/dmp/mappers/dmpDocumentHelpers'
import { toNhrPatientInformation } from '../business/mappers/toNhrPatientInformation'
import { useNationalHealthRecords } from './useNationalHealthRecords'
import { useInvalidateNhrDocuments } from './useInvalidateNhrDocuments'
import type { NhrActionModalConfig } from '../types/nhrDocumentActions'

type ModalState = {
  doc: NhrDocument
  config: NhrActionModalConfig
}

const archiveConfig = (): NhrActionModalConfig => ({
  title: i18n.t('national_health_records.document_metadata_actions.dmp.dialogs.archive.title'),
  body: i18n.t('national_health_records.document_metadata_actions.dmp.dialogs.archive.body'),
  confirmLabel: i18n.t('national_health_records.document_metadata_actions.dmp.dialogs.archive.confirm'),
})

const unarchiveConfig = (): NhrActionModalConfig => ({
  title: i18n.t('national_health_records.document_metadata_actions.dmp.dialogs.unarchive.title'),
  body: i18n.t('national_health_records.document_metadata_actions.dmp.dialogs.unarchive.body'),
  confirmLabel: i18n.t('national_health_records.document_metadata_actions.dmp.dialogs.unarchive.confirm'),
})

type UseNhrDocumentStatusActionsResult = {
  options: DropdownActionOption[]
  modalState: ModalState | null
  isLoading: boolean
  error: string | null
  handleConfirm: () => Promise<void>
  handleDismiss: () => void
}

export const useNhrDocumentStatusActions = ({ patient }: { patient: Patient }): UseNhrDocumentStatusActionsResult => {
  const nhrOperations = useNationalHealthRecords({ patient })
  const invalidateNhrDocuments = useInvalidateNhrDocuments(patient)

  const [modalState, setModalState] = useState<ModalState | null>(null)
  const [isLoading, setIsLoading] = useState(false)
  const [error, setError] = useState<string | null>(null)

  const openModal = useCallback(
    (doc: NhrDocument, config: NhrActionModalConfig) => {
      setError(null)
      setModalState({ doc, config })
    },
    // setError and setModalState are stable useState setters — no deps needed

    []
  )

  const options: DropdownActionOption[] = useMemo(
    () => [
      {
        key: 'archive',
        label: i18n.t('national_health_records.document_metadata_actions.dmp.options.archive'),
        isAvailable: isActive,
        onClick: (doc) => openModal(doc, archiveConfig()),
      },
      {
        key: 'unarchive',
        label: i18n.t('national_health_records.document_metadata_actions.dmp.options.unarchive'),
        isAvailable: isArchived,
        onClick: (doc) => openModal(doc, unarchiveConfig()),
      },
    ],
    [openModal]
  )

  const handleConfirm = async () => {
    if (!modalState || !nhrOperations || !patient.ins) return

    setIsLoading(true)
    setError(null)

    try {
      await nhrOperations.updateDocumentMetadata({
        patient: toNhrPatientInformation(patient.ins),
        params: { payloads: [{ id: modalState.doc.id, propertyName: 'status' }] },
      })
      setModalState(null)
      await invalidateNhrDocuments()
    } catch {
      setError(i18n.t('national_health_records.document_metadata_actions.dmp.errors.action_failed'))
    } finally {
      setIsLoading(false)
    }
  }

  const handleDismiss = () => {
    if (isLoading) return
    setModalState(null)
    setError(null)
  }

  return { options, modalState, isLoading, error, handleConfirm, ha

…(truncated, 2574 more chars)

Turn 1 — Assistant

I'll review the new files in PR #241005 for simplification opportunities. Let me first check the actual file paths in the repo to give accurate references.

Tools used:

- Bash: find /Users/USER/workspace/doctolib/doctolib -type f \( -name "useNhrDocumentStatusActions.ts" -o -name "useInvalidateNhrDocuments.ts" -o -name "NhrDocumentActionDialog.tsx" \) 2>/dev/null | head
…(truncated, 4 more chars)

Tools used:

- Bash: find /Users/USER/workspace/doctolib/doctolib -type d -name "national-health-records*" 2>/dev/null | head -10

Tool results:

  → result (ok): (Bash completed with no output)

Tool results:

  → result (ok): /Users/USER/workspace/doctolib/doctolib/packages/@doctolib/national-health-records
/Users/USER/workspace/doctolib/doctolib/.claude/worktrees/fhir-2144_nhr-document-list-add-action-config-column/packages/@doctolib/national-health-records
/Users/USER/workspace/doctolib/doctolib/.claude/worktrees/crazy-volhard/packages/@doctolib/national-health-records

I have enough context from the diff snippets. Let me provide concrete simplification findings for the PR. The files weren't found in the local repo (likely on a feature branch), so I'll reference the file names as supplied in the PR description.

Code Simplification Review — PR #241005

1. Redundant isOpen prop in NhrDocumentActionDialog.tsx

File: NhrDocumentActionDialog.tsx (Props type and component body)
Issue: The component has two redundant null/open guards: isOpen={modalState !== null} is computed at the call site, then if (!config) return null re-checks the same condition (config is only set when modalState is non-null in the hook). The isOpen prop becomes load-bearing on the consumer keeping both states in sync.

Suggested simplification: Drop the isOpen prop entirely — config already encodes the open/closed state.

type Props = {
  config: NhrActionModalConfig | null
  isLoading: boolean
  error: string | null
  onConfirm: () => void
  onDismiss: () => void
}

export const NhrDocumentActionDialog = ({ config, isLoading, error, onConfirm, onDismiss }: Props) => {
  if (!config) return null

  return (
    <Dialog
      show
      title={config.title}
      ...

And at the call site in PatientCard.tsx:
<NhrDocumentActionDialog
  config={modalState?.config ?? null}
  isLoading={isLoading}
  error={error}
  onConfirm={handleConfirm}
  onDismiss={handleDismiss}
/>

---

2. Unnecessary useCallback + misplaced comment in useNhrDocumentStatusActions.ts

File: useNhrDocumentStatusActions.ts (openModal definition, ~lines around the useCallback block)
Issue: Three problems compound:
- openModal is wrapped in useCallback only because it's referenced in options' useMemo deps — but the options array is rebuilt every render anyway since i18n.t(...) calls aren't reactive and the array isn't passed to a memoized child that depends on referential identity.
- The explanatory comment sits inside the deps-array argument list (between ] and )), which is syntactically valid but visually awkward.
- useMemo on options provides little value here — it returns a small inline array with no expensive work.

Suggested simplification: Inline openModal and drop the useMemo (or keep useMemo but lose useCallback):

const openModal = (doc: NhrDocument, config: NhrActionModalConfig) => {
  setError(null)
  setModalState({ doc, config })
}

const options: DropdownActionOption[] = [
  {
    key: 'archive',
    label: i18n.t('national_health_records.document_metadata_actions.dmp.options.archive'),
    isAvailable: isActive,
    onClick: (doc) => openModal(doc, archiveConfig()),
  },
  {
    key: 'unarchive',
    label: i18n.t('national_health_records.document_metadata_actions.dmp.options.unarchive'),
    isAvailable: isArchived,
    onClick: (doc) => openModal(doc, unarchiveConfig()),
  },
]

If memoization is genuinely needed downstream, keep useMemo with [] deps (since i18n.t reads at render and setModalState/setError are stable) — but don't introduce useCallback solely to satisfy a deps array.

---

3. Over-engineered archiveConfig / unarchiveConfig factories

File: useNhrDocumentStatusActions.ts (top-of-file factory functions)
Issue: Two single-call factory functions exist only to defer i18n.t(...) evaluation until click time. They add ~10 lines and a layer of indirection for what is essentially a static literal.

Suggested simplification: Build the configs inline at the click site (already evaluated lazily because onClick runs on user action):

{
  key: 'archive',
  label: i18n.t('national_health_records.document_metadata_actions.dmp.options.archive'),
  isAvailable: isActive,
  onClick: (doc) => openModal(doc, {
    title: i18n.t('national_health_records.document_metadata_actions.dmp.dialogs.archive.title'),
    body: i18n.t('national_health_records.document_metadata_actions.dmp.dialogs.archive.body'),
    confirmLabel: i18n.t('national_health_records.document_metadata_actions.dmp.dialogs.archive.confirm'),
  }),
},

Alternatively, if you want to avoid that visual density, keep one helper:

const dialogConfig = (kind: 'archive' | 'unarchive'): NhrActionModalConfig => ({
  title: i18n.t(national_health_records.document_metadata_actions.dmp.dialogs.${kind}.title),
  body: i18n.t(national_health_records.document_metadata_actions.dmp.dialogs.${kind}.body),
  confirmLabel: i18n.t(national_health_records.document_metadata_actions.dmp.dialogs.${kind}.confirm),
})

That collapses 12 lines to 6 and preserves type safety.

---

4. useInvalidateNhrDocuments returns a fresh function on every render

File: useInvalidateNhrDocuments.ts (entire body)
Issue: The returned closure is a new reference on every render. Any consumer that puts it in a deps array (useEffect, useCallback, useMemo) will fire every render. This is a latent footgun.

Suggested simplification: Wrap in useCallback:

import { useCallback } from 'react'
import { useQueryClient } from '@tanstack/react-query-v5'
import type { Patient } from 'monolith_DEPRECATED/types/doctor/api/Patient'

export function useInvalidateNhrDocuments(patient: Patient): () => Promise<void> {
  const queryClient = useQueryClient()
  return useCallback(
    () => queryClient.invalidateQueries({ queryKey: ['dmp-documents', patient.ins] }),
    [queryClient, patient.ins],
  )
}

This also adds an explicit return type per project standards (CLAUDE.md: "Use explicit return type annotations for top-level functions") and switches to the function keyword — the file currently uses an arrow-function export, which conflicts with the project's "Prefer function keyword over arrow functions" rule. The same applies to useNhrDocumentStatusActions and NhrDocumentActionDialog (both are exported const arrow functions and should use function declarations).

---

5. handleConfirm / handleDismiss not stable references

File: useNhrDocumentStatusActions.ts (return value)
Issue: Both handlers are recreated every render. If NhrDocumentActionDialog ever wraps in React.memo or these are passed to useEffect deps, this triggers churn. Lower priority than #4 because they're currently consumed inline by a non-memoized child, but it's inconsistent with the useCallback already used for openModal.

Suggested approach: Either drop the useCallback from openModal (per #2) for consistency, or wrap handleConfirm/handleDismiss in useCallback to match. Pick one convention.

---

6. error reset on dismiss is dead code while modal is open

File: useNhrDocumentStatusActions.ts (handleDismiss)
Issue:
const handleDismiss = () => {
  if (isLoading) return
  setModalState(null)
  setError(null)
}

setError(null) here clears state on close, but openModal also calls setError(null). One of them is redundant. Keeping the reset on openModal is sufficient and avoids a transient render where the dialog is closing with both config and error momentarily out of sync.

Suggested simplification: Drop setError(null) from handleDismiss:
const handleDismiss = () => {
  if (isLoading) return
  setModalState(null)
}

---

7. ModalState shape stores doc but never reads it after open

File: useNhrDocumentStatusActions.ts (ModalState type and handleConfirm)
Issue: handleConfirm reads modalState.doc.id but nothing else ever reads modalState.doc. The only field actually consumed downstream is the id. This is fine as-is for type clarity, but worth noting: if NhrDocument ever becomes expensive to retain (e.g., contains large blobs), prefer storing only the id. No change required unless that's a concern.

---

Summary of recommended edits (priority order)

| # | File | Change | Why |
|---|------|--------|-----|
| 1 | NhrDocumentActionDialog.tsx | Remove 
…(truncated, 909 more chars)