Project: adapter-observability · Branch: .invalid · Mode: ` · **Model:**claude-opus-4-7`
Stats: 1 user messages, 2 tool calls, tools used: Read.
Conversation
Turn 1 — User
Review the changed code in PR #244702 for simplification opportunities. Focus only on the 2 changed files:
-
/Users/USER/workspace/doctolib/doctolib/.claude/worktrees/bug_fhir-2290-fix-syntaxerror-nhr-adapter-observability/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.ts- Changed lines 89–90:?.length ?? 0→!= null ? .length : 0 -
/Users/USER/workspace/doctolib/doctolib/.claude/worktrees/bug_fhir-2290-fix-syntaxerror-nhr-adapter-observability/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.test.ts- Added one new test case
The fix must NOT use ?. + ?? together (that's the bug). Please read both files and suggest any simplifications while respecting that constraint. Report only actionable suggestions.
Turn 1 — Assistant
I'll review both files for simplification opportunities while respecting the constraint that ?. + ?? cannot be used together.
Tools used:
Read:/Users/USER/workspace/doctolib/doctolib/.claude/worktrees/bug_fhir-2290-fix-syntaxerror-nhr-adapter-observability/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.ts
Tools used:
Read:/Users/USER/workspace/doctolib/doctolib/.claude/worktrees/bug_fhir-2290-fix-syntaxerror-nhr-adapter-observability/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.test.ts
Tool results:
→ result (ok): 1 import type {
2 NhrAdapter,
3 NhrDocumentSearchProps,
4 NhrDocumentSearchResult,
5 NhrGetDocumentsContentProps,
6 NhrGetDocumentsContentResult,
7 NhrResultStatus,
8 } from '@doctolib/tpf-national-health-records'
9 import { sendNhrRequest, sendNhrResponse } from './nhrMonitoring'
10 import type {
11 NhrAdapterName,
12 NhrAdapterAction,
13 NhrRequestDetails,
14 NhrResponseDetails,
15 NhrRequestActionFields,
16 NhrResponseActionFields,
17 } from '../../types'
18
19 // Pha
…(truncated, 4659 more chars)
Tool results:
→ result (ok): 1 import { sendMonitoringEvent } from 'monolith_DEPRECATED/services/monitoring'
2 import type { NhrAdapter, NhrDocumentSearchProps } from '@doctolib/tpf-national-health-records'
3 import { withNhrObservability } from './NhrAdapterWithObservability'
4
5 jest.mock('monolith_DEPRECATED/services/monitoring', () => ({
6 sendMonitoringEvent: jest.fn().mockResolvedValue(undefined),
7 }))
8
9 const mockSendMonitoringEvent = sendMonitoringEvent as jest.MockedFunction<typeof sendMonitoringEvent>
10
1
…(truncated, 21888 more chars)
I've reviewed both files. Here are my actionable findings.
Summary
The fix on lines 89-90 is correct and respects the constraint (no ?. + ?? combo). The added regression test at lines 190-202 is well-placed with a clear FHIR-2290 reference comment explaining the bug.
Actionable Simplification Suggestions
1. Eliminate the asymmetric guard pattern on lines 89-90 (NhrAdapterWithObservability.ts)
The current pattern is verbose and inconsistent with the codebase's existing style for similar checks (lines 91-92 use !! for booleans; lines 93-98 use || for date filters).
Current (lines 89-90):
categories_count: props.params.categories != null ? props.params.categories.length : 0,
formats_count: props.params.formats != null ? props.params.formats.length : 0,
Suggested simplification (uses a local destructure + plain ternary, no ?. + ?? combo):
const { categories, formats } = props.params
// ...
categories_count: categories ? categories.length : 0,
formats_count: formats ? formats.length : 0,
Or, even more uniform with the surrounding ternaries (and avoiding the repeated property access that the original != null form has):
categories_count: props.params.categories ? props.params.categories.length : 0,
formats_count: props.params.formats ? props.params.formats.length : 0,
Notes:
- A truthy check (categories ? ...) is safe here: arrays are always truthy when defined, so the only falsy case is undefined/null — exactly the case we want to map to 0.
- This is consistent with the !!props.params.status style used on the very next line.
- Avoids the != null double-negative phrasing that is slightly less idiomatic in this file.
Trade-off note: if the team prefers explicit null-checking (more defensive against future type changes where categories could be e.g. number | undefined), the current != null form is fine — just keep it. The main win available is the local destructure to remove the doubled props.params.categories / props.params.formats access.
2. Optional: extract a tiny helper for the repeated length-or-zero pattern
If you anticipate adding more _count fields for optional array params in the future, a tiny helper kills the duplication:
function countOrZero(arr: readonly unknown[] | null | undefined): number {
return arr ? arr.length : 0
}
// usage
categories_count: countOrZero(props.params.categories),
formats_count: countOrZero(props.params.formats),
I would only do this if a 3rd similar field is on the horizon. For just two call sites it is over-engineering — leave it inline.
3. Test file — no changes recommended
NhrAdapterWithObservability.test.ts is well-structured:
- The new regression test (lines 190-202) is correctly scoped to the empty-array case, which is the precise behavior that would silently regress if someone reintroduced ?.length ?? 0 and the transpilation gap re-appeared.
- The FHIR-2290 comment at line 190 satisfies the traceability rule.
- The split between "absent" (lines 180-188) and "explicitly empty" (lines 191-202) is meaningful — both paths now exercise the fixed code.
No simplifications worth making here.
Relevant Files
/Users/USER/workspace/doctolib/doctolib/.claude/worktrees/bug_fhir-2290-fix-syntaxerror-nhr-adapter-observability/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.ts(lines 89-90)/Users/USER/workspace/doctolib/doctolib/.claude/worktrees/bug_fhir-2290-fix-syntaxerror-nhr-adapter-observability/packages/@doctolib/national-health-records/src/business/adapters/NhrAdapterWithObservability.test.ts(no changes recommended)