OpenConceptLab/ocl_issues#2522 | target_repo.version is mandatory | removed fallback HEAD

[snyaggarwal]

Linked Issue

ClosesOpenConceptLab/ocl_issues#2522

[paynejd]

Moving quickly so sending claude feedback directly with minimal curation.

Verdict: approve with minor questions

Hits all four acceptance criteria:

• Save gated by hasSelectedTargetRepoVersion (button-disabled + onSave alert) — ConfigurationForm.jsx:502, MapProject.jsx:1211.
• Legacy-project gate: load path pops the configure drawer + shows error alert when version missing — MapProject.jsx:715-775.
• buildProjectContext returns null when version unset, so no downstream caller can read a fabricated version.
• Both legacy defaults gone: || 'HEAD' at MapProject.jsx:1060 and || undefined at :693 (now getTargetRepoVersionFromUrl / getProjectTargetRepoVersion).

Strengths

• New projectTargetRepo.js helper is small, pure, and well-unit-tested (6 cases incl. blanks, fallback to target_repo.source_version, versionless URL).
• onSave no longer writes target_repo_url conditionally — required-by-contract now matches required-by-code.
• RepoVersionSearchAutocomplete gains error/helperText props for inline form feedback (only shown when a repo is picked — correct UX).

Questions / things to confirm before merging

1. Removal of auto-pick fallbacks in fetchVersions (MapProject.jsx:1920-1928). Previously, when no selected version was passed, it auto-picked the only version, then the only released version. Both fallbacks are gone — new-project users must click the version dropdown even when there's literally one valid choice.

   The intent of #2522 is that repo version is always explicit rather than implicit — versionless repo identifiers should never be used to request content. But that does NOT preclude the UI helping users make the selection simple and intuitive via transparent defaults the user can override. Suggest restoring an auto-pick of the only-released version (or the only version) but rendering it as a clearly-presented default selection in the dropdown — the user can see what was picked and adjust before save. The thing to avoid is silent HEAD-or-equivalent fallbacks deeper in the code, which this PR already fixes.

2. URIToParentParams still hardcodes 'HEAD' default in utils.js:758. MapProject's two old call sites are gone, but the foot-gun remains for future callers. Either drop the default, or add a code comment that callers must validate the result.

3. setConfigure(Boolean(!loadProjectContext)) opens the drawer whenever projectContext is null (missing canonical OR missing version). For a brand-new project (no target_repo_url), the drawer auto-opens on load — probably what you want, but worth a sanity check that this doesn't double-open with the existing new-project flow.

4. selectedTargetRepoVersion dependency in buildProjectContext callback (replaces repoVersion). If a parent re-fetches a richer repoVersion object with the same id (e.g. metadata enrichment), the memoized context won't refresh. Today nothing in ctx depends on extra version metadata, so this is fine — flagging only if future fields like repoVersion.match_algorithms need to land in the context.

5. Test coverage is helper-only — no test exercises the onSave gate or the load-time configure-drawer pop. Acceptable given the repo's existing test posture, but worth noting.

Nits

• Save guard does if(!repoVersion?.version_url || !selectedTargetRepoVersion) — version_url and id come from the same dropdown object; the double check is defensive but redundant.
• i18n key map_project.target_repo_version_required reused in two places — fine, but the form helper text and the toast read identically; a load-time variant ("after save attempt") might help users distinguish.

[snyaggarwal]

@paynejd

1. Fixed — restored transparent version auto-selection in MapProject.jsx. When the repo has exactly one version, or exactly one released version, we now preselect it in the dropdown so the user can still see and change it. This keeps selection explicit in the UI without bringing back hidden fallback behavior.

2. Fixed — removed the 'HEAD' default from utils.js. URIToParentParams(..., true) now leaves repoVersion unset when the URL is versionless, so future callers do not silently inherit a fabricated version.

3. No change — The current setConfigure(Boolean(!loadProjectContext)) behavior is still intentional: on saved-project load, if the project context is incomplete, we open configuration so the user can fix it. It never double-opens.

4. No change -- Future-proofing concern is valid, but today buildProjectContext only uses the selected version id, not richer version metadata, so the current dependency on selectedTargetRepoVersion is still correct for this PR.

5. Added more tests.

[paynejd]

Re-review of ce7648e ("addressing PR review comments"). Both substantive points resolved; ready to merge from my side.

Point #1 — auto-pick defaults: ✅ resolved

New getDefaultTargetRepoVersion(versions) helper restores transparent defaults:

• 1 version → pick it
• Multiple versions but exactly 1 released → pick the released one
• Multiple released, or no released among many → no default, user picks

Slightly more conservative than the pre-PR behavior (which find()-ed the first released even when several existed) — but the original "first released" was arbitrary array order anyway, so forcing an explicit pick when the default is ambiguous is the right call. Selected version shows up in the dropdown, so the user sees what was picked and can override — meets the "transparent, adjustable defaults" bar from #2522.

Nice factoring: kept the policy in the pure helper, added 3 unit tests covering all branches, fetchVersions is now a one-liner.

Point #2 — URIToParentParams HEAD default: ✅ resolved

parts[5] || 'HEAD' → parts[5]. Verified safe by grepping all call sites: the only caller that ever passed includeVersion=true was the MapProject code paths this PR already removed. Every other consumer (Associations.jsx, MappingDecisionResult.jsx, FromAndTargetSource.jsx, SearchFilters.jsx) omits the second arg, so repoVersion isn't set on their return value — no behavioral change anywhere.

Outstanding (low priority, not blocking)

• Points [Snyk] Security upgrade lodash from 4.17.21 to 4.17.23 #3–[Snyk] Security upgrade axios from 1.13.4 to 1.13.5 #5 from the previous review were sanity-checks / observations, not asks. Leaving them as-is.
• The two nits (redundant version_url + id save guard, duplicate i18n key) also untouched — fine to defer.

LGTM. 🚀

[paynejd]

Approving per re-review above — both substantive points addressed.