Repository navigation
DO NOT MERGE -- OpenConceptLab/ocl_issues#2922 | Add to Collection names the repo version being browsed in its expressions - #94
Conversation
…sion being browsed in its expressions The dialog built its expressions with dropVersion(c.url) and had no idea which repo version was selected, so the API resolved them to the latest source version whatever version the user was browsing. The concept list, card view and concept header now pass in the repo being browsed. toRepoVersionURI puts its version in the expression when it is a version of the resource's own source. On HEAD, in global search and from a collection the expression stays version-less, as it is now. The API call preview shows the expressions the dialog sends, also when several concepts are selected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AaZ4qUrDxUHWvsX34Npdnf
paynejd
left a comment
There was a problem hiding this comment.
Codex review, pass 1 (codex-cli 0.166.0, commit b96b803)
Clean.
findings.json
{
"reviewer": "codex-cli 0.166.0",
"perspective": "defects",
"mode": "full",
"commit": "b96b8036df981a0dba5e25e6852bfc0380614aa4",
"verdict": "clean",
"prior": [],
"findings": []
}
paynejd
left a comment
There was a problem hiding this comment.
Simplification review (commit b96b803)
8 cuts.
S1 · reuse · src/components/common/AddToCollectionDialog.jsx:80 · The repo prop threaded through six components, since contextRepo in OperationsContext already holds the repo being browsed → the dialog reads contextRepo; the six other component files leave the diff (-5)
S2 · shrink · src/common/uri.js:28 · resourcePath and the concepts/mappings test, which only matter for a URI that already names a repo version → resourceURI.replace(dropVersion(repo.version_url), repo.version_url) (-6)
S3 · dup-test · src/common/tests/uri.test.js:33 · The user-owned source assertion, which takes the same path as the concept assertion → nothing replaces it (-4)
S4 · dup-test · src/common/tests/uri.test.js:41 · The concepts/v1/ and concepts/a%2Fb/ assertions, which take the same path → nothing replaces them; the test title loses "whatever the ids repeat" (-2)
S5 · dup-test · src/common/tests/uri.test.js:50 · The false and {} assertions, which take the same branch as undefined → nothing replaces them (-2)
S6 · dup-test · src/common/tests/uri.test.js:58 · The MySource2 and Other assertions, which fail the same check as the collection → nothing replaces them (-2)
S7 · dup-test · src/common/tests/uri.test.js:68 · The empty-string assertion, which takes the same branch as undefined → nothing replaces it (-1)
S8 · shrink · src/components/common/CascadeSelector.jsx:41 · previewExpressions, whose only job is the placeholder → a default parameter on expressions (-1)
findings.json
{"reviewer":"claude subagent","perspective":"simplify","mode":"full","commit":"b96b8036df981a0dba5e25e6852bfc0380614aa4","verdict":"findings","prior":[],"findings":[{"id":"S1","severity":null,"tag":"reuse","file":"src/components/common/AddToCollectionDialog.jsx","line":80,"claim":"Cut the `repo` prop threaded through six components: `contextRepo` in OperationsContext (src/components/app/LayoutContext.jsx) already holds the repo being browsed. RepoHome sets it to the same object as its `repo` state for a source (RepoHome.jsx:169), clears it on unmount (RepoHome.jsx:343) and remounts per repo (App.jsx:262), and Search.jsx:439 already reads it for `target_repo_url`. It is false in global search and in a collection, where the props end up version-less too, so the expressions come out the same. The cut covers Search.jsx:804, SearchResults.jsx:440 and :479, CardResults.jsx:6 and :28, ConceptCard.jsx:13 and :78, ConceptHome.jsx:384, and ConceptHeader's `browsedRepo` with its comment (ConceptHeader.jsx:24-26 and :175). The ticket's 'passed in from the caller' is its starting point, not its expected behavior.","scenario":null,"action":"In AddToCollectionDialog: `import { OperationsContext } from '../app/LayoutContext'`, `const { contextRepo } = React.useContext(OperationsContext)` and `toRepoVersionURI(c.url, contextRepo)`. The six other component files leave the diff.","lines_delta":-5,"confidence":"high"},{"id":"S2","severity":null,"tag":"shrink","file":"src/common/uri.js","line":28,"claim":"Cut `resourcePath` and the `/^(concepts|mappings)\\//` test. They only matter for a URI under the repo that isn't followed by concepts/ or mappings/, meaning one that already names a repo version, and `c.url` is never that: oclapi2's concept serializers return `versioned_object_url`, which is `drop_version(uri)`, with `uri` built from the HEAD source, and the ticket's step 3 shows the same. Every caller passes concepts only (SearchResults.jsx:283). Run side by side on 120 URI and repo pairs, the shorter form differs only on that case. Not checked against a live response. The test at uri.test.js:62-64 goes with it.","scenario":null,"action":"Lines 28-30 become `return resourceURI.replace(dropVersion(repo.version_url), repo.version_url)`; no match leaves the URI as it is. A `$` can't trip the replacement, since the API percent-encodes version ids in `version_url`.","lines_delta":-6,"confidence":"medium"},{"id":"S3","severity":null,"tag":"dup-test","file":"src/common/__tests__/uri.test.js","line":33,"claim":"Cut the third assertion, the user-owned source on version 2.0. It takes the same path as line 31 with different strings: the helper never reads the owner type or the version's format, and line 17 already covers /users/ in dropVersion.","scenario":null,"action":"nothing replaces it","lines_delta":-4,"confidence":"high"},{"id":"S4","severity":null,"tag":"dup-test","file":"src/common/__tests__/uri.test.js","line":41,"claim":"Cut the `concepts/v1/` and `concepts/a%2Fb/` assertions. Both take line 31's path: the helper never compares an id with the version and never decodes, and line 12 covers repeated ids in dropVersion.","scenario":null,"action":"nothing replaces it; the test title loses 'whatever the ids repeat'","lines_delta":-2,"confidence":"high"},{"id":"S5","severity":null,"tag":"dup-test","file":"src/common/__tests__/uri.test.js","line":50,"claim":"Cut the `false` and `{}` assertions: both take the same `!repo?.version_url` branch as `undefined` on line 49.","scenario":null,"action":"nothing replaces it","lines_delta":-2,"confidence":"high"},{"id":"S6","severity":null,"tag":"dup-test","file":"src/common/__tests__/uri.test.js","line":58,"claim":"Cut the `MySource2` and `Other` assertions. Both fail the same `startsWith` as COLLECTION_V1 on line 57, and `MySource2` doesn't pin the slash boundary, since `.../MySource2/` could never prefix `.../MySource/concepts/A/`.","scenario":null,"action":"nothing replaces it","lines_delta":-2,"confidence":"high"},{"id":"S7","severity":null,"tag":"dup-test","file":"src/common/__tests__/uri.test.js","line":68,"claim":"Cut the `''` assertion: it takes the same `!resourceURI` branch as `undefined` on line 67.","scenario":null,"action":"nothing replaces it","lines_delta":-1,"confidence":"high"},{"id":"S8","severity":null,"tag":"shrink","file":"src/components/common/CascadeSelector.jsx","line":41,"claim":"Cut `previewExpressions`: its only job is a placeholder when no expressions come in, and AddReferencesDialog, the one caller that passes none, gets that from a default parameter. An empty list would then preview as empty, which happens only when the dialog has nothing to send (AddToCollectionDialog.jsx:134).","scenario":null,"action":"`const CascadeSelector = ({ onChange, expressions = ['/:owner/sources/:source/concepts/:id/'], collectionUrl, showPreviewDefault = false }) => {` and `{ data: { expressions }, cascade: request.cascade }`.","lines_delta":-1,"confidence":"medium"}]}…alog reads the source version from contextRepo, fewer tests OperationsContext's contextRepo already holds the source being browsed, and search's $match already reads its version_url, so the repo prop through six components goes. It is false in global search and from a collection, so expressions stay version-less there as before. toRepoVersionURI loses the guard for a URI that already names a repo version: the API's concept url never does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AaZ4qUrDxUHWvsX34Npdnf
|
Response to the simplification review S1 applied in 352e10f. Only RepoHome sets |
What changed and why
TBv3 scopes concepts by the repo version in the URL, but the Add to Collection dialog built its expressions with
dropVersion(c.url)and had no idea which repo version was selected. The API got no version, so it resolved the reference to the latest source version, whatever version the user was browsing. A concept retired in the latest version could end up in the collection as the retired concept.Now, while browsing a version of a source, the expression names that version, in the API call preview and in the request:
/orgs/MyOrg/sources/MySource/concepts/A/→/orgs/MyOrg/sources/MySource/v1/concepts/A/toRepoVersionURI(uri, repo)insrc/common/uri.jsbuilds the expression. It uses the version only when the repo is a version of the resource's own source.contextRepoinOperationsContext, which covers all its callers: the concept list's bulk action, card view and the concept header. The ticket's starting point was to pass the version in from each caller, and the first commit did that. The simplification review replaced it:RepoHomealready keeps this value and search's$matchalready reads itsversion_url, so six components don't need a new prop./:owner/sources/:source/concepts/:id/placeholder, which would have hidden the version.How it was tested
PR checks (
pr.yml, Node 22):npm cipassed,./node_modules/eslint/bin/eslint.js src --ext .jsx,.jspassed,npm testpassed (119 tests).--no-eslintrc -c .eslintrc.json) it passes there too.Unit tests:
src/common/__tests__/uri.test.jsadds 5 tests. With the baseuri.jsthose 5 fail (114 pass, 5 fail); with the change, all 119 pass.Browser check: headless Chromium with a fresh context at 1600×1000, and once at 1280×800. The app was served by
npm startagainst the staging API, anonymously and read-only. The session was faked in the browser with no sign-in, and the script answered thePUTitself, so nothing reached staging. It drove the real dialog and compared the expressions in the API call preview with those in the capturedPUT.Before the change, the bulk action, concept header and card view on
v2026-08-24all sent the version-less/orgs/CIEL/sources/CIEL/concepts/170891/. On the final code, all 21 scenarios gave the expected expression (one was rerun alone, after fixing how the script picked its concept):PUTv2026-08-24, not the latest: bulk action, one concept/orgs/CIEL/sources/CIEL/v2026-08-24/concepts/170891/v2026-08-24, in the preview too/orgs/CIEL/sources/CIEL/v2026-08-24/concepts/170891/v2026-07-20, then bulk action or concept headerv2026-07-20v2026-07-20's panel tov2026-08-24's, with the panel still mountedv2026-08-24v2026-09-24/orgs/TO/sources/test-source/concepts/MA/v2026-08-24to a collection version, then adding a CIEL concept/orgs/CIEL/sources/CIEL/concepts/168691/v2026-08-24to global search, then adding a CIEL concept/orgs/CIEL/sources/CIEL/concepts/112141/v2026-08-24to global search, then the concept headerv2026-08-24to another source's HEADI looked at the screenshots of the dialog, the preview and the results list. This change adds no strings, so no locale needed work.
Acceptance criteria
v2026-08-24).toRepoVersionURIhandles mapping URIs and is tested on them.Docs
No doc describes how these expressions are built, so none changed. The repo has no
docs/folder, the README doesn't cover this flow, andchangelog.mdhasn't been edited since the first commit.Review
startsWithandsliceinstead ofreplace, S5 keeps thefalseassertion rather thanundefined, and S6 keeps one assertion for another source.No finding is open.
For the reviewer
The latest release now gets a versioned expression too. Opening a source lands on its latest release, and the URL names it, so that's indistinguishable from choosing the version. By the ticket's rule ("browsing a specific repo version") the expression names it. A reference added that way stays on that version when the source releases a newer one, where before it was a dynamic reference. If that default should stay dynamic, it's a small change, but it needs the ticket's rule to say so.
Chose not to do
contextRepoisn't set on a collection's page, and a collection's version says nothing about the version of the concept's source. So adding from a collection still sends a version-less expression, which resolves to the latest source version: a CIEL concept shown in a collection atv2026-03-23is sent as/orgs/CIEL/sources/CIEL/concepts/168691/. Whether that should name the concept's own source version instead is a question the ticket doesn't settle.Noticed, not fixed
CloneToSourceDialogalso builds its expressions withdropVersion(item.url), so it doesn't send the repo version being browsed either. I didn't test what the clone then resolves to. It's a separate flow from this ticket.onClickin the React tree. The screenshots show the same panel before and after this change.v2026-07-20's panel to 170891's panel onv2026-08-24, where that concept exists, fetched/orgs/CIEL/sources/CIEL/v2026-07-20/concepts/170891/, got a 404 and showed the error. So the panel fetched with the version just left. This change doesn't touch that code; I didn't run this on the base branch.addToCollection.adding_singlekey exists, and the results summary builds its text and plurals in English.closes OpenConceptLab/ocl_issues#2922
🤖 Generated with Claude Code
https://claude.ai/code/session_01AaZ4qUrDxUHWvsX34Npdnf
Written by an Octoad session worker (
worker-v0.17, recipelean-loop, flowpublic>public, runopenconceptlab-ocl-issues-2922-20261104-093906).