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

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

Conversation

Turn 1 — User

Review PR https://github.com/doctolib/doctolib/pull/241055 — "feat(FHIR-2195): add archive/unarchive actions to document list".

The PR wires Archive/Unarchive actions into the Document List UI. Key files changed:

---
packages/@doctolib/national-health-records/src/hooks/useNhrDocumentStatusActions.ts (NEW)
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,

…(truncated, 6374 more chars)

Turn 1 — Assistant

I'll review the PR by examining the code changes provided. Let me analyze each file carefully for the issues mentioned.

Tools used:

- Bash: ls /Users/USER/workspace/doctolib/ 2>/dev/null | head -20

Tool results:

  → result (ok): actions
billeo-engine
c3-rest-api-guidelines
docto-crypt
doctoboot
doctolib
doctolib-pr-review.skill
doctolib-pro-desktop
github-workspaces
kube
misc
oxygen
prompts
software-design-training
terraform-aws-secrets
terraform-infra
test-failures
ti-client
treatment-plan
unified-healthcare-data-platform-infra

I have all the information I need from the PR diff provided. Let me analyze the code carefully against the review criteria.

PR Review: FHIR-2195 — Add archive/unarchive actions to document list

Reviewing
- useNhrDocumentStatusActions.ts (NEW)
- NhrDocumentActionDialog.tsx (NEW)
- useInvalidateNhrDocuments.ts (NEW)
- DmpNhrAdapter.ts (MODIFIED)
- dmpDocumentHelpers.ts (MODIFIED)
- variants/types.ts (MODIFIED)
- index.ts (MODIFIED)
- PatientCard.tsx (MODIFIED)
- DmpApi.ts (MODIFIED)

---

Critical (90-100)

issue (critical): Wrong relative import path in NhrDocumentActionDialog.tsx — confidence: 98
File: packages/@doctolib/national-health-records/src/components/NhrDocumentActionDialog/NhrDocumentActionDialog.tsx:4

import type { NhrActionModalConfig } from '../types/nhrDocumentActions'

The component is nested at src/components/NhrDocumentActionDialog/NhrDocumentActionDialog.tsx. The types file lives at src/types/nhrDocumentActions, so the correct relative path is ../../types/nhrDocumentActions (two levels up). The current path resolves to src/components/NhrDocumentActionDialog/types/nhrDocumentActions, which doesn't exist. This will fail TypeScript compilation and break the build.

Fix:
import type { NhrActionModalConfig } from '../../types/nhrDocumentActions'

---

issue (critical): uuid is never passed in the hook's updateDocumentMetadata call — confidence: 95
File: packages/@doctolib/national-health-records/src/hooks/useNhrDocumentStatusActions.ts:75-77

await nhrOperations.updateDocumentMetadata({
  patient: toNhrPatientInformation(patient.ins),
  params: { payloads: [{ id: modalState.doc.id, propertyName: 'status' }] },
})

The adapter (DmpNhrAdapter.ts) destructures uuid from each payload and passes it to dmpApi.updateDocumentStatus({ ins, documentUniqueId: id, uuid: uuid ?? '' }). Because the hook never forwards modalState.doc.uuid, the API is always called with uuid: '', which likely breaks DMP server-side validation (DMP CDA documents are correlated by uniqueId + uuid/setId for status transitions). The result type even includes uuid as part of the per-payload echo, suggesting it's contractually required.

Fix:
params: { payloads: [{ id: modalState.doc.id, uuid: modalState.doc.uuid, propertyName: 'status' }] },

(Also confirm uuid exists on NhrDocument; if the field is named differently — setId, versionId, etc. — use the correct one.)

---

issue (critical): patient.ins accessed before adapter conversion — toNhrPatientInformation(patient.ins) is suspicious — confidence: 88
File: packages/@doctolib/national-health-records/src/hooks/useNhrDocumentStatusActions.ts:75

if (!modalState || !nhrOperations || !patient.ins) return
// ...
patient: toNhrPatientInformation(patient.ins),

toNhrPatientInformation is typically a mapper that takes a Patient (or its INS subobject) and returns NhrPatientInformation. Passing only patient.ins (a string-typed identifier or a partial object) likely strips identity fields (firstName, lastName, birthDate, gender) that DMP requires for the auth/sign step. Verify the mapper signature — if it expects the full Patient, the call must be toNhrPatientInformation(patient). Today this likely sends a malformed payload to DMP. Calling it confidence 88 because I can't see the mapper signature, but this is a strong red flag worth blocking on.

---

Important (80-89)

issue (important): handleConfirm swallows error details — confidence: 90
File: packages/@doctolib/national-health-records/src/hooks/useNhrDocumentStatusActions.ts:80-83

} catch {
  setError(i18n.t('national_health_records.document_metadata_actions.dmp.errors.action_failed'))
}

The catch block discards the underlying error entirely (no catch (e), no logging, no Sentry/Datadog reporting). For a feature that mutates patient health records on a national registry, silent failure is a serious observability gap. There is also no per-payload status: 'error' handling — updateDocumentMetadata returns { status: 'error', message } for individual payloads (as seen in DmpNhrAdapter.ts), but the hook treats every promise resolution as success. A "successful" promise with all payloads in status: 'error' will close the modal and invalidate queries as if the action had succeeded.

Fix:
} catch (e) {
  reportError(e) // or logger.error
  setError(i18n.t('national_health_records.document_metadata_actions.dmp.errors.action_failed'))
}

And also inspect the response payload results before treating the operation as a success:
const response = await nhrOperations.updateDocumentMetadata(...)
const failed = response.payloads.find((p) => p.status === 'error')
if (failed) { setError(...); return }

---

issue (important): handleConfirm is not wrapped in useCallback — stale closure / re-render churn — confidence: 82
File: packages/@doctolib/national-health-records/src/hooks/useNhrDocumentStatusActions.ts:71

const handleConfirm = async () => { ... }
const handleDismiss = () => { ... }

Both handlers are recreated on every render. They are returned from the hook and likely passed as props to <NhrDocumentActionDialog> (via PatientCard), causing unnecessary re-renders and breaking referential-equality optimizations downstream. openModal is wrapped in useCallback but handleConfirm/handleDismiss are not — be consistent.

Fix: Wrap both with useCallback listing the actually-read deps (modalState, nhrOperations, patient.ins, invalidateNhrDocuments, isLoading).

---

issue (important): useInvalidateNhrDocuments returned function is not memoised — invalidates referential identity each render — confidence: 85
File: packages/@doctolib/national-health-records/src/hooks/useInvalidateNhrDocuments.ts:6-8

return () => queryClient.invalidateQueries({ queryKey: ['dmp-documents', patient.ins] })

The returned closure is rebuilt on every render. Any consumer that puts it in a useEffect dep array or useCallback dep array (including useNhrDocumentStatusActions if you wrap handleConfirm properly) will re-run on every render. Also, the query key ['dmp-documents', patient.ins] should match the keys actually used by the document-fetching hook(s); if those hooks key by patient.id, patient.ins.identifier, or include filter args after the prefix, this invalidation is either incomplete or misses entirely.

Fix:
return useCallback(
  () => queryClient.invalidateQueries({ queryKey: ['dmp-documents', patient.ins] }),
  [queryClient, patient.ins]
)

Also verify the actual query key shape used by the producer query.

---

issue (important): total?: number becoming optional is a breaking change for consumers — confidence: 85
File: packages/@doctolib/national-health-records/src/variants/types.ts

interface SearchDocumentsResult {
  total?: number  // was required, now optional
}

Relaxing a required field to optional is technically backward-compatible at the producer side, but every consumer that previously read result.total as number now needs a ?? 0 or equivalent. TS strict will flag those at the call sites — and if the type is exported across package boundaries, downstream packages may break. Confirm all result.total usages in the monorepo were updated, and document the rationale (some adapters can't return total?). Otherwise revert to total: number and have the new code path return total: documents.length when unknown.

---

issue (important): DmpApi.updateDocumentStatus throws if frDmpWorkflow missing — but adapter expects { success, error } — confidence: 88
File: packages/@doctolib/dmp/src/business/DmpApi.ts

`
…(truncated, 4468 more chars)