Repository navigation
OpenConceptLab/ocl_online#655 | Show the versioning mode in version details - #97
snyaggarwal wants to merge 3 commits into
Conversation
snyaggarwal
left a comment
There was a problem hiding this comment.
No actionable findings in this frontend change.
snyaggarwal
left a comment
There was a problem hiding this comment.
Second review: the head SHA is unchanged and I found no actionable findings.
snyaggarwal
left a comment
There was a problem hiding this comment.
Third-round review
The head is unchanged from the previous review (6442269a). I rechecked the PR in the context of the updated backend contract and found no new actionable findings, performance issues, or nits.
…n place of the versioning mode
…ded-from test so spellcheck passes
paynejd
left a comment
There was a problem hiding this comment.
Claude review, with a Codex pass (commit 01a2fc4)
Verdict: approved with this fix. Apply the suggestion below (or an equivalent) and add the test, and it's good to merge after oclapi2#985. "Seeded From" works as D7 in the Repo Versions Model proposes. I ran this branch against production LOINC with seeded_from added to the API responses in the browser. An import-seeded version shows "Import", a version made from HEAD shows "HEAD", and HEAD shows the version the import seeded. The labels are in all four languages, and the row never shows raw JSON for the shapes oclapi2#985 writes today.
Fix before merge
- [P2]
src/components/repos/versionsTab.styles.js:134: a seed with no label crashes the page.getSeededFromLabelreturnsundefinedfor{"type": "repo_version", "url": null}and for any type it doesn't know.EntityAttributesDialogthen falls back toget(entity, field), gets the object back and, with notype, renders it as a React child. React throws "Objects are not valid as a React child", and the whole page shows "Something went wrong." I reproduced this by giving one LOINC version{"type": "repo_version", "url": null}. The migration in oclapi2#985 writes that shape for a version whose source has no HEAD, and later seed types (ocl_online#678) would hit it too, since TBv3 and the API deploy separately. The new tests check the helper's return value but not the dialog, so they pass. @snyaggarwal: there's a one-click suggestion inline that leaves the row out when there's no label. Please add a test throughgetVersionAttributeFieldswith a null URL and an unknown type. Codex found this on its own and rates it P1. I'd call it P2 because nothing on production has that shape today, but it's a one-line fix for a whole-page crash.
Nit
- [P3] The PR title still says "versioning mode".
How it was checked
- Read OpenConceptLab/ocl_online#673, the diff against
main(it merges cleanly) andEntityAttributesDialog. CI is green. - Ran this branch's dev build against the production API, read-only, with
seeded_fromadded to version responses in the browser: HEAD, an import-seeded version, a version made from HEAD, and one with no URL. - Codex (codex-cli 0.169.0, xhigh), one pass on the whole diff: 1 finding, the one above.
findings.json
[
{
"id": "W1",
"severity": "P2",
"file": "src/components/repos/versionsTab.styles.js",
"line": 134,
"claim": "Seeds without a display label crash the attributes dialog.",
"scenario": "For the migration shape {\"type\":\"repo_version\",\"url\":null}, or an unknown seed type returned by a newer API, getSeededFromLabel returns undefined. EntityAttributesDialog then uses info?.value || get(entity, field), recovering the original object. With the JSON formatter disabled, React throws 'Objects are not valid as a React child'. This was reproduced through the dialog; the helper-only tests pass without exercising this fallback.",
"action": "Prevent fallback to the seed object when no label exists, and add a dialog-path test covering null-URL and unknown-type seeds.",
"confidence": "high",
"source": "codex (F1.1, rated P1) + claude, reproduced in a dev build"
}
]| if(field === 'seeded_from') | ||
| info.value = getSeededFromLabel(value, t); | ||
| fields[field] = info; |
There was a problem hiding this comment.
[P2] With no label, EntityAttributesDialog falls back to get(entity, field), gets the object back and React throws, so the whole page shows "Something went wrong." This leaves the row out instead. I checked it in the dev build with {"type": "repo_version", "url": null}: the dialog opens without the row, and ESLint is clean.
| if(field === 'seeded_from') | |
| info.value = getSeededFromLabel(value, t); | |
| fields[field] = info; | |
| if(field === 'seeded_from') { | |
| info.value = getSeededFromLabel(value, t); | |
| // with no label the dialog falls back to the raw object, which React can't render | |
| if(!info.value) | |
| return; | |
| } | |
| fields[field] = info; |
OpenConceptLab/ocl_online#655, OpenConceptLab/ocl_online#673
What this does
This shows Seeded From in a version's details on the Versions tab: HEAD, the version it was seeded from (for example 2.82), or Import. The value comes from the new
seeded_fromfield in oclapi2. The dialog never shows the field as raw JSON; a seed it doesn't recognise shows nothing.src/components/repos/seededFrom.js: turnsseeded_frominto that label.src/components/repos/versionsTab.styles.js: addsseeded_fromto the version attribute labels and order.Testing
npm test: 125 tests pass, including 3 new ones for the label.eslintis clean on the changed files.The backend part is in OpenConceptLab/oclapi2 on the branch of the same name.
🤖 Generated with Claude Code