Skip to content

Unified mapper data model PR1: normalizers + parallel state#16

Open
paynejd wants to merge 2 commits intomainfrom
issues#2337
Open

Unified mapper data model PR1: normalizers + parallel state#16
paynejd wants to merge 2 commits intomainfrom
issues#2337

Conversation

@paynejd
Copy link
Copy Markdown
Member

@paynejd paynejd commented May 6, 2026

Summary

PR1 of the row-scoped, canonical-URL-identified candidate/concept refactor for OCL Mapper. Introduces the new entity model (AlgorithmResponse, Candidate, ConceptDefinition, ConceptRow) and the normalizer that produces it, behind a feature flag that is OFF by default. Reads still come from legacy allCandidates — flipping reads is PR2.

Spec: unified-mapper-model.md — full design rationale, key decisions, and implementation status table.

References OpenConceptLab/ocl_issues#2337 (does not yet close — PR1 of 3).

What's in this PR

New files:

  • src/components/map-projects/normalizers.js — pure normalizeAlgoResult / normalizeAlgorithmInvocation. Resolves canonical ConceptReference {url, code, version?} per algorithm's concept_identity config (target_repo / bridge_repo / fixed). Produces the four entities with intra-invocation dedup (richer ConceptDefinition wins).
  • src/components/map-projects/conceptKey.js — opaque pool key helpers. Avoids the FHIR canonical |version collision that would arise from concatenating url|code.
  • src/components/map-projects/__tests__/normalizers.test.js — 26 unit tests (Node node:test, no new framework dependency).

Modified:

  • src/components/map-projects/algorithms.jsxconcept_identity block on ocl-search and ocl-semantic.
  • src/components/map-projects/MapProject.jsxUNIFIED_MODEL_ENABLED feature flag (default OFF), parallel rowMatchState hook, mergeIntoRowMatchState callback, buildProjectContext (reads repo.canonical_url or derives https://ns.openconceptlab.org{url}), flag-gated normalize-and-merge in onResponse (success + failure paths).
  • package.jsonnpm test script using built-in node --test.

Verification

  • npm test → 26/26 passing
  • npm run eslint → clean
  • npm run build → webpack compiled successfully (after npm install to sync recent package updates)
  • Smoke: with the flag off, runtime behavior is unchanged

Test plan

  • Code review the normalizer + tests against the spec linked above
  • CI: npm install, npm test, npm run eslint, npm run build all green
  • Smoke test: load Mapper, run candidates on an existing project, save/reload — should be indistinguishable from main
  • (Optional) Flag-on dev session: temporarily set UNIFIED_MODEL_ENABLED = true locally, run candidates, inspect rowMatchState in React DevTools, capture one real $match response and confirm shape matches the synthesized fixtures in the test file. Don't merge with the flag on.

Deferred to follow-up PRs

  • PR 2 — flip reads (Candidates.jsx, Concept.jsx, Score.jsx, SearchHighlightsDialog.jsx, AICandidatesAnalysis.jsx, MapButton.jsx); wire bridge and scispacy code paths into the normalizer; add canonical_url field to custom-algo config UI; surface canonical context in ConfigurationForm.jsx; rebuild $lookup as ensureLoaded over $resolveReference; remove match_type.
  • PR 3 — flip writes; remove legacy allCandidates; ship schema-v2 save format with normalizeLegacy.js for backward-compatible load.

🤖 Generated with Claude Code

…lizers + parallel state

Introduces the normalized candidate/concept entity model (plans/unified-mapper-model.md)
behind a feature flag, in parallel with the legacy allCandidates state. Reads still
come from legacy state — flipping reads is PR2.

- normalizers.js: pure normalizeAlgoResult / normalizeAlgorithmInvocation; resolves
  canonical ConceptReference {url, code, version?} per algo's concept_identity config
  (target_repo / bridge_repo / fixed); produces AlgorithmResponse + Candidate +
  ConceptDefinition + ConceptRow with intra-invocation dedup.
- conceptKey.js: opaque pool key helpers (avoids the FHIR canonical |version
  collision that would arise from concatenating url|code).
- __tests__/normalizers.test.js: 26 tests covering conceptKey roundtrip, standard /
  scispacy / bridge / multi-cascade normalization, multi-algo convergence on shared
  canonical reference, and intra-invocation dedup.
- algorithms.jsx: concept_identity block on ocl-search and ocl-semantic.
- MapProject.jsx: UNIFIED_MODEL_ENABLED feature flag (default OFF), parallel
  rowMatchState hook + ref, mergeIntoRowMatchState callback, buildProjectContext
  (reads repo.canonical_url or derives https://ns.openconceptlab.org{url}),
  flag-gated normalize-and-merge in onResponse success and failure paths.
- package.json: npm test script using built-in node:test (no new test framework
  dependency).

PR1 verified: npm test 26/26 passing, npm run eslint clean, npm run build succeeds.
With the flag off, runtime behavior is unchanged.

Co-Authored-By: Claude Opus 4 (1M context) <noreply@anthropic.com>
concept_rows: { ...prevRow.concept_rows },
}

for(const cr of concept_rows) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forEach is our convention. for feels dated

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 10d0940 — converted to forEach. Also applied across the other for...of loops introduced in this PR for consistency.

// richer (lookup_status='full') over stubs ('pending'/'partial').
setConceptCache(prev => {
const next = { ...prev }
for(const def of concept_definitions) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use forEach

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 10d0940.

// Target repo canonical URL is read from repo metadata; if absent, derive
// 'https://ns.openconceptlab.org' + relative URL (per OCL canonical
// conventions — see plans/unified-mapper-model.md).
const buildProjectContext = () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to be outside the scope of fetchAllCandidatesForRow

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 10d0940 — hoisted to component scope as a React.useCallback next to mergeIntoRowMatchState, with deps [project, owner, repo, repoVersion]. Reusable from the PR2 read paths.

mergeIntoRowMatchState(__row.__index, normalizeAlgorithmInvocation(null, {
algorithmId: algoId,
algorithmConfig: algoDef,
projectContext: buildProjectContext(),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buildProjectContext() can be extracted on top before if block and must be reused in both block if/else

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 10d0940projectContext is now computed once at the top of onResponse (gated by UNIFIED_MODEL_ENABLED) and reused in both the failure and success branches.

mergeIntoRowMatchState(__row.__index, normalizeAlgorithmInvocation(rowPayload, {
algorithmId: algoId,
algorithmConfig: algoDef,
projectContext: buildProjectContext(),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reuse from prev

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 10d0940 — same hoisted projectContext const reused here.

const defsByKey = new Map()
const rowsByKey = new Map()

for (const result of results) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use forEach

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 10d0940 — converted all four for...of loops in normalizers.js to forEach.

…t buildProjectContext

Address Sunny's review comments on PR #16:
- Convert all for...of loops introduced in this PR to forEach (project convention)
- Hoist buildProjectContext from fetchAllCandidatesForRow to component-level useCallback
- Compute projectContext once in onResponse and reuse across success/failure branches

No behavior change. Feature flag remains OFF by default.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@paynejd paynejd requested a review from snyaggarwal May 8, 2026 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants