Skip to content

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

Open
snyaggarwal wants to merge 3 commits into
mainfrom
issues#2522
Open

OpenConceptLab/ocl_issues#2522 | target_repo.version is mandatory | removed fallback HEAD#27
snyaggarwal wants to merge 3 commits into
mainfrom
issues#2522

Conversation

@snyaggarwal
Copy link
Copy Markdown
Contributor

Linked Issue

Closes OpenConceptLab/ocl_issues#2522

@snyaggarwal snyaggarwal requested a review from paynejd May 18, 2026 03:30
@paynejd
Copy link
Copy Markdown
Member

paynejd commented May 25, 2026

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
Copy link
Copy Markdown
Contributor Author

@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.

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.

OCL Mapper | Require explicit target_repo.version in project config form (no blank, no silent HEAD default)

2 participants