Skip to content

PR3-C: v2-only oclmap (drop allCandidates + legacy load/save)#35

Draft
paynejd wants to merge 3 commits into
mainfrom
pr3c-v2-only-oclmap
Draft

PR3-C: v2-only oclmap (drop allCandidates + legacy load/save)#35
paynejd wants to merge 3 commits into
mainfrom
pr3c-v2-only-oclmap

Conversation

@paynejd
Copy link
Copy Markdown
Member

@paynejd paynejd commented May 27, 2026

Summary

Closes OpenConceptLab/ocl_issues#2541. Companion to the v1->v2 migration script in oclmap#34.

Draft until the v1->v2 migration runs against prod in the maintenance window. Until then, this code reads empty for every project (no `mapper_schema_version: 2` marker on existing blobs).

After the migration + this PR ship together, oclmap reads/writes only the v2 wire format. ~−267 LOC across `MapProject.jsx` + `normalizers.js` + one deleted test file.

What changes

Change Effect
`onSave` writes v2 directly (`mapper_schema_version: 2` + `concept_definitions` + `rows`) Replaces the `_getCandidates` flatten of `allCandidates`
`fetchAndSetProject` deserializes v2 directly into `rowMatchState` + `conceptCache` Replaces the legacy `_allCandidates` hydration + `normalizeLegacyAllCandidates` backfill
Deleted: post-load re-normalize-on-canonical-resolve effect Was a workaround for the legacy load missing the live canonical; v2 load captures it at save time
Deleted: `allCandidates` state + `allCandidatesRef` + ~10 `setAllCandidates` writes Single source of truth is `rowMatchState`
Translated: 49 `allCandidatesRef.current` readers Read from `rowMatchState` + `conceptCache` directly, or via the small `derivedAllCandidates()` projection helper for CSV-export readers
Deleted: `normalizeLegacyAllCandidates` from `normalizers.js` + its test file Wire format is v2 only
Deleted: `UNIFIED_MODEL_ENABLED` flag Always true since PR2b
Net −267 LOC

Test plan

  • `npm test` — 135/135 (the 12 legacy-path tests were in the deleted `normalizeLegacyAllCandidates.test.js`).
  • `npm run eslint` clean on touched files.
  • `npm run build` compiles successfully.
  • Browser smoke test before merge (during the maintenance window):
    • Load an existing migrated v2 project — candidates render identically vs. pre-migration.
    • Run a per-row match — results appear in the candidates list and quality view.
    • Save — the new candidates blob in the DB is v2-shaped.
    • Reload — state restored identically.
    • CSV export — produces the expected per-algo columns with scores.
    • Bridge cascade rendering — bridge_child rows show cascade-target details.
    • AI Assistant — buildV2RecommendationPayload still produces the same payload shape (no fallback path; reads only from `rowMatchState`).

Notes for review

  • The `derivedAllCandidates()` helper at MapProject.jsx ~L3078 is intentional pragmatism: it produces the legacy `{algoId: [{row, results}, ...]}` shape from `rowMatchState` + `conceptCache` on demand, so CSV-export readers don't have to be refactored in this PR. The whole helper is a candidate for follow-up cleanup once each reader is rewritten to consume `rowMatchState` directly.
  • The migration script (oclmap#34) imports `normalizeLegacyAllCandidates`. This PR deletes that function — so the script becomes non-functional after this merges. The script's job is one-shot (the maintenance-window migration). Its README documents this lifecycle.

🤖 Generated with Claude Code

paynejd and others added 3 commits May 27, 2026 07:26
…allCandidates + legacy load/save

After the v1->v2 candidates migration (ocl_issues#2540) runs in the
maintenance window, oclmap reads/writes only the v2 wire format
(mapper_schema_version: 2 + concept_definitions + rows).

Changes:
- onSave: serialize rowMatchState + conceptCache directly to v2.
  No more legacy candidates array.
- fetchAndSetProject: deserialize v2 directly into rowMatchState +
  conceptCache. No more normalizeLegacyAllCandidates round-trip.
- Deleted: post-load re-normalize-on-canonical-resolve effect,
  allCandidates state, allCandidatesRef, setAllCandidates writes
  across the bulk-match/bulk-bridge/bulk-scispacy/per-row/onResponse
  paths.
- Deleted: normalizeLegacyAllCandidates from normalizers.js +
  its test file.
- Deleted: UNIFIED_MODEL_ENABLED flag (always true after PR2b).
- Translated 49 allCandidates callsites to read from rowMatchState +
  conceptCache. A small derivedAllCandidates() helper preserves the
  v1-shape projection for CSV-export readers that haven't yet been
  refactored to consume rowMatchState directly.

Net -267 LOC.

DO NOT MERGE until the migration script (oclmap#34) has been run
against prod. Until then, this code reads empty for every project.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Member Author

@paynejd paynejd left a comment

Choose a reason for hiding this comment

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

Review of the two follow-up commits (21f42b7, 7e857a5). Both are correct and fix real bugs — setConfigure(false) removes a dangling loadProjectContext ReferenceError on project load, and the conceptBelongsToTargetRepo relative-URL fallback recovers candidates whose migrated/derived canonical no longer equals the live target_repo.canonical_url. Tests 136/136, eslint clean.

Four polish suggestions below (none blocking). A and D are quick wins; B+C align an inconsistent guard and fix a now-false comment.

return url.endsWith('/') ? url : `${url}/`
}
const normalizeCanonicalUrl = (url) => {
if(typeof url !== 'string') return ''
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.

A — empty-string canonicals compare equal (latent). normalizeCanonicalUrl('') returns '/', not '', which defeats the !== '' guard in canonicalUrlsEqual — so canonicalUrlsEqual('', '') is true, and two concepts with empty canonical URLs are wrongly treated as target-repo members. Verified by exercising the helper directly. Mirror normalizeRelativeUrl's empty-string guard; after this the two helpers are identical and could be merged into one.

Suggested change
if(typeof url !== 'string') return ''
if(typeof url !== 'string' || url === '') return ''

if(canonicalUrlsEqual(conceptDefinition?.reference?.url, targetCanonical)) return true
const relativeUrl = normalizeRelativeUrl(targetRelativeUrl)
const oclUrl = conceptDefinition?.ocl_url || ''
return relativeUrl !== '' && typeof oclUrl === 'string' && oclUrl.startsWith(relativeUrl)
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.

D — dead check. oclUrl is already coalesced to a string by || '' on the line above, so typeof oclUrl === 'string' is always true. Drop it.

Suggested change
return relativeUrl !== '' && typeof oclUrl === 'string' && oclUrl.startsWith(relativeUrl)
return relativeUrl !== '' && oclUrl.startsWith(relativeUrl)

Comment on lines 224 to 226
// ConceptDefinition.reference.url, so the comparison is exact.
const isTargetRepoView = (view) => {
if(!targetCanonical) return true
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.

B — stale comment + C — guard inconsistency. This comment now contradicts the code: the comparison is no longer exact — that's the whole point of conceptBelongsToTargetRepo's relative-URL fallback. And this guard checks only !targetCanonical, whereas the sibling guards in qualityRowViews, Concept.isMappable, and pickTopRowView use !targetCanonical && !targetRelativeUrl. So if canonical is ever absent but relative_url is present, the table Action column would render MapButtons on every row (including bridge intermediaries). Align both:

Suggested change
// ConceptDefinition.reference.url, so the comparison is exact.
const isTargetRepoView = (view) => {
if(!targetCanonical) return true
// ConceptDefinition.reference.url in the common case; the comparison also
// tolerates canonical drift via the OCL relative-URL fallback below.
const isTargetRepoView = (view) => {
if(!targetCanonical && !targetRelativeUrl) return true

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.

PR3-C: v2-only oclmap (drop allCandidates + legacy load/save)

2 participants