Project: workspace-doctolib · Branch: HEAD · Mode: ` · **Model:**claude-sonnet-4-6`
Stats: 1 user messages, 8 tool calls, tools used: Read, Bash.
Conversation
Turn 1 — User
Review the recently modified TypeScript hook files in the monolith for FHIR-2182 server-side pagination compliance with Doctolib UpFront architecture patterns.
Files to review:
- /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2182_server-side-pagination-hook/packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.ts
- /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2182_server-side-pagination-hook/packages/@doctolib/national-health-records/src/variants/types.ts
Context: These are hook-layer files in packages/@doctolib/national-health-records/src/hooks/ (a Doctolib TypeScript package). The hook returns an async function (not a React hook with state). The change adds a page parameter and slices the React Query cached result.
Please check for: 1. Naming conventions and file structure compliance 2. Separation of concerns 3. Any UpFront anti-patterns
If the files are backend/hook-layer code that doesn't map to UpFront component patterns, note that and hold off from applying frontend-specific rules.
Turn 1 — Assistant
I'll start by reading both files to understand the current implementation before applying the review framework.
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2182_server-side-pagination-hook/packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.ts
Tool results:
→ result (ok): 1 import { useQueryClient } from '@tanstack/react-query-v5' 2 import type { Patient } from 'monolith_DEPRECATED/types/doctor/api/Patient' 3 import { isFeatureEnabled } from '@doctolib/variation_management' 4 import i18n from 'monolith_DEPRECATED/services/i18n' 5 import type { NhrDocumentListVariant, MapFiltersToSearchDocumentsParams, ServerFilters } from '../variants/types' 6 import { useNationalHealthRecords } from './useNationalHealthRecords' 7 import { generateNhrDocuments } from '../mocks/ge …(truncated, 3491 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2182_server-side-pagination-hook/packages/@doctolib/national-health-records/src/variants/types.ts
Tool results:
→ result (ok): 1 import type { ColumnConfig, FilterConfig, SearchDocumentsResult } from '@doctolib/tpf-national-health-records-ui'
2 import type { Patient } from 'monolith_DEPRECATED/types/doctor/api/Patient'
3 import type { NhrDocumentSearchParams } from '@doctolib/tpf-national-health-records'
4
5 export type { SearchDocumentsResult }
6
7 export type NhrDocumentListVariant = {
8 columns: ColumnConfig[]
9 filters: FilterConfig[]
10 searchDocuments: (appliedFilters?: Record<string, unknown>, page?: numb
…(truncated, 863 more chars)
Now let me gather additional context about the package structure to complete the review.
Tools used:
Bash:find /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2182_server-side-pagination-hook/packages/@doctolib/national-health-records/src -type f | sort
Tool results:
→ result (ok): /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2182_server-side-pagination-hook/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.test.ts /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2182_server-side-pagination-hook/packages/@doctolib/national-health-records/src/business/adapters/DmpNhrAdapter.ts /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2182_server-side-pagination-hook/packages/@doctolib/national-health-records/src/business/adapters/pa …(truncated, 8124 more chars)
Tools used:
Bash:ls /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2182_server-side-pagination-hook/packages/@doctolib/national-health-records/
Tool results:
→ result (ok): babel.config.cjs catalog-info.yaml CODEOWNERS.local i18n.json jest.config.cjs package.json src tsconfig.json
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2182_server-side-pagination-hook/packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.test.ts
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2182_server-side-pagination-hook/packages/@doctolib/national-health-records/src/index.ts
Tool results:
→ result (ok): 1 export { useNationalHealthRecords, type UseNhrOperations } from './hooks/useNationalHealthRecords' 2 export { useDmpNhr } from './hooks/useDmpNhr' 3 export { useNhrDocumentListVariant } from './variants/useNhrDocumentListVariant' 4 export type { DocumentSearchOptions, NhrDocumentListVariant } from './variants/types' 5 export { actionsBuilder, type NhrActionsBuilder } from './business/builders/actionsBuilder' 6 export { useGetDocumentsContent } from './hooks/useGetDocumentsContent' 7 export { u …(truncated, 719 more chars)
Tool results:
→ result (ok): 1 import React from 'react' 2 import { renderHook } from '@testing-library/react' 3 import { QueryClient, QueryClientProvider } from '@tanstack/react-query-v5' 4 import * as variationManagement from '@doctolib/variation_management' 5 import type { Patient } from 'monolith_DEPRECATED/types/doctor/api/Patient' 6 import type { NhrDocument, NhrDocumentSearchParams } from '@doctolib/tpf-national-health-records' 7 import { useNationalHealthRecords } from './useNationalHealthRecords' 8 import { generat …(truncated, 28229 more chars)
Now let me check the git diff to understand what was actually changed in this branch versus base.
Tools used:
Bash:git diff origin/master...HEAD -- packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.ts packages/@doctolib/national-health-records/src/variants/types.ts 2>/dev/null | head -200
Tool results:
→ result (ok): diff --git a/packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.ts b/packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.ts index 8678e58f13ac..c081510fb599 100644 --- a/packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.ts +++ b/packages/@doctolib/national-health-records/src/hooks/useSearchDocuments.ts @@ -10,6 +10,7 @@ import { toNhrPatientInformation } from '../business/mappers/toNhrPatientInforma
const DMP_DOCUMENTS_STALE_TIME_ …(truncated, 3542 more chars)
Tools used:
Read:/Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2182_server-side-pagination-hook/packages/@doctolib/national-health-records/src/hooks/useNationalHealthRecords.ts
Tool results:
→ result (ok): 1 import type {
2 NhrDocumentSearchProps,
3 NhrDocumentSearchResult,
4 NhrGetDocumentsContentProps,
5 NhrDocumentContentResult,
6 NhrDocumentMetadataUpdateProps,
7 NhrDocumentMetadataUpdateResult,
8 } from '@doctolib/tpf-national-health-records'
9 import { useProductLineVariation, ProductLineKey } from '@doctolib/variation_management'
10 import type { Patient } from 'monolith_DEPRECATED/types/doctor/api/Patient'
11 import { useDmpNhr } from './useDmpNhr'
12
13 export type UseNhrOper
…(truncated, 966 more chars)
I now have a complete picture of both files, the diff, and the package structure. Here is the review.
Architecture Review: FHIR-2182 Server-Side Pagination
Preliminary Note on Scope
Both files live in packages/@doctolib/national-health-records/src/ which is a TypeScript package in the monolith, not an UpFront frontend-only package. The reviewed hook (useSearchDocuments) is a shared business hook — it couples React Query state management with domain logic — and is correctly placed in src/hooks/. UpFront frontend-specific rules (Container/Presentational split, UI suffix, SCSS modules) do not apply here. The review focuses on the rules that do apply: file placement, naming, separation of concerns, export conventions, and internal code quality.
Critical Issues (must fix)
1. PAGE_SIZE constant is not exported and duplicated across files
PAGE_SIZE = 20 is defined privately in useSearchDocuments.ts (line 15) with a code comment stating it matches a constant in @doctolib/tpf-national-health-records-ui and will be replaced once 16.2.0 ships. The test file (useSearchDocuments.test.ts, line 426) re-declares const PAGE_SIZE = 20 independently. This is a magic-number duplication: if the value changes, there are now two places to update in this package alone, and the test will silently diverge from production behaviour.
The comment says "replace with a re-export from @doctolib/tpf-national-health-records-ui once 16.2.0 ships" but there is no tracking mechanism (no TODO, no ticket reference). This is a latent maintenance debt.
Recommended fix: Export PAGE_SIZE from useSearchDocuments.ts (or a shared constants file) so the test file can import it rather than redeclare it:
// useSearchDocuments.ts
export const PAGE_SIZE = 20 // TODO(FHIR-2182): replace with re-export from @doctolib/tpf-national-health-records-ui once 16.2.0 ships
// useSearchDocuments.test.ts
import { PAGE_SIZE } from './useSearchDocuments'
If PAGE_SIZE is not intended for public consumption outside the hook, at minimum deduplicate it via a shared internal constants file within src/hooks/ or src/business/constants/.
2. page is excluded from the React Query cache key — this is architecturally unsound as currently structured
The comment at line 54–57 of useSearchDocuments.ts explains the intent: exclude page from the query key so that all pages for a given filter set share one cache entry and slicing happens in-process. This is a deliberate and valid trade-off, but it has a critical correctness risk that is not guarded against.
The hook calls queryClient.fetchQuery with staleTime: DMP_DOCUMENTS_STALE_TIME_MS (5 minutes). The cached value is the full unsliced document list. However, the hook returns the sliced result from the closure's page variable. If a caller independently calls queryClient.invalidateQueries or if any other part of the codebase forces a refetch (e.g., useInvalidateNhrDocuments), the full list is refetched, but page is not part of the cache key so no consumer component is re-rendered by React Query's own mechanisms — consumers only get the new data on their next explicit searchDocuments() call. This is acceptable for an imperative searchDocuments callback pattern, but the hook provides no documentation-level contract warning about this.
More concretely: the useInvalidateNhrDocuments hook exists at /Users/USER/workspace/doctolib/worktrees/doctolib/fhir-2182_server-side-pagination-hook/packages/@doctolib/national-health-records/src/hooks/useInvalidateNhrDocuments.ts. If its query key prefix overlaps with ['dmp-documents', ...], it will invalidate the full-list cache but leave callers unaware unless they re-call. This coupling is invisible in the current code.
Recommended fix: Add a comment directly above the queryKey line documenting the invalidation contract — specifically that useInvalidateNhrDocuments callers must trigger a re-fetch of the current page manually if they rely on fresh data, and that this is a known accepted trade-off.
Warnings (should fix)
3. src/hooks/ houses a hook that mixes data access with business/pagination logic
Per UpFront separation rules, src/data/ is for data hooks (repositories) and src/business/ is for shared business logic. src/hooks/ is for platform API hooks (useOffline, useCopyToClipboard) and custom UX hooks.
useSearchDocuments does all three: it accesses React Query directly (useQueryClient), applies business pagination logic (slicing with PAGE_SIZE), and orchestrates the NHR adapter. In the UpFront taxonomy this belongs in src/business/ (shared business hook that mixes data with logic) rather than src/hooks/.
This is not a new violation introduced by FHIR-2182 — the hook was already in src/hooks/ before this change — but the pagination addition increases the amount of business logic inside the hook and makes the misplacement more significant. This should be tracked for a follow-up migration if not addressed now.
4. currentPage is not present in the SearchDocumentsResult type — or its absence is unverified
types.ts re-exports SearchDocumentsResult from @doctolib/tpf-national-health-records-ui (line 5). The hook now always sets currentPage: page on every return path. If SearchDocumentsResult does not include currentPage in its type definition, TypeScript will either widen the return type silently or produce a type error depending on how the upstream type is defined.
The review cannot confirm the shape of SearchDocumentsResult without reading the upstream package, but the test file asserts currentPage on every response shape, which implies it must be present on the type. Verify that SearchDocumentsResult in @doctolib/tpf-national-health-records-ui includes currentPage?: number or currentPage: number. If it does not, useSearchDocuments is returning an object that exceeds the declared return type contract, and TypeScript may be silently discarding the extra field at the call site.
Run npm run type-check in the package to confirm there are no suppressed type errors here.
5. The mock path path imports generateNhrDocuments and filterMockedDocuments directly in production hook code
Lines 7–8 of useSearchDocuments.ts:
import { generateNhrDocuments } from '../mocks/generateNhrDocuments'
import { filterMockedDocuments } from '../mocks/filterMockedDocuments'
These imports exist in the production hook file, guarded only by a runtime feature flag check. This means mock generation code is bundled into the production artifact for every environment. This pattern is widespread in the codebase (not introduced by this PR), but the FHIR-2182 change extends the mock path with the pagination slice, deepening the coupling.
This is acceptable if the team has a deliberate policy of feature-flag-gated mocks in production bundles, but if tree-shaking or bundle size is a concern, the mock imports should be moved behind a dynamic import or isolated to a dev-only variant.
Suggestions (consider improving)
6. NhrDocumentListVariant.searchDocuments signature uses Record<string, unknown> instead of ServerFilters
In types.ts line 10, the searchDocuments signature uses Record<string, unknown> for appliedFilters. ServerFilters is defined in the same file (line 20). The internal cast appliedFilters as ServerFilters in useSearchDocuments.ts lines 32 and 61 compensates for this loose typing. This is an existing issue, not introduced by FHIR-2182, but tightening the signature would eliminate the unsafe casts:
searchDocuments: (appliedFilters?: ServerFilters, page?: number) => Promise<SearchDocumentsResult>
7. Test file declares PAGE_SIZE inline with a magic number
Addressed above in Critical Issue 1, but worth repeating in isolation: the test-level const PAGE_SIZE = 20 at line 426 of the test file is a
…(truncated, 2005 more chars)