Repository navigation
OpenConceptLab/ocl_issues#2916 | A reopened project restores a mapped target's names and match highlight from its saved matches - #104
Conversation
…ach mapped target's names and match highlight from its saved matches Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… match per row, and escape highlights in Match Details - getConcept returns a copy of the cached concept with the row's own names and match (withCachedConcept), instead of writing them into the shared entry - the row panel keeps the target's own names and highlight over a candidate's (withOwnNames) - Match Details renders highlights through highlightHtml, as the matched name does Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Codex review, pass 1 (codex-cli 0.163.0, commit 0645163)
3 findings: 3 P1. (F1.1's wording is shortened here; the fix is in the response below.)
- F1.1 · P1 · src/components/map-projects/viewBuilders.js:466 · Restored highlights reach Match Details, which renders them as HTML without escaping. (A saved matches entry holds a highlight with markup other than . Reopening the project and opening the mapped target's Match Details passes it through buildPanelPayload to MatchDetailsPanel, which renders it as HTML. getMatchedName's escaping covers only the inline matched name.)
- F1.2 · P1 · src/components/map-projects/tests/views.test.js:779 · The round-trip test bypasses onSave, which still overwrites distinct highlights for rows sharing a cached target. (Reopen two rows mapped to the same concept with different saved highlights, then select a row so onCSVRowSelect caches that concept by URL. During onSave, getConcept returns and mutates the same cached object for both rows. Both serialized matches consequently contain the last row's highlight. Executing the actual getConcept and save-selection code reproduced this loss; the test directly serializes restored instead.)
- F1.3 · P1 · src/components/map-projects/MapProject.jsx:1446 · Restored names and highlights are still overwritten when composing the row-panel target. (A Search-mapped target also appears among the row's algorithm candidates, such as a semantic candidate without highlights. Loading restores the saved highlight, but the targetConcept construction at lines 5350–5365 replaces its metadata with the candidate projection. The row panel loses the saved highlight; a candidate definition lacking names also replaces the restored localized name with the default name. Executing that construction reproduced both losses while the grid retained the restored values.)
findings.json
{
"reviewer": "codex-cli 0.163.0",
"perspective": "defects",
"mode": "full",
"commit": "064516361cfe3121be049cb1e274b38ea0a11f5d",
"verdict": "findings",
"prior": [],
"findings": [
{
"id": "F1.1",
"severity": "P1",
"tag": null,
"file": "src/components/map-projects/viewBuilders.js",
"line": 466,
"claim": "Restored highlights reach Match Details, which renders them as HTML without escaping.",
"scenario": "A saved matches entry holds a highlight with markup other than <em>. Reopening the project and opening the mapped target's Match Details passes it through buildPanelPayload to MatchDetailsPanel, which renders it as HTML. getMatchedName's escaping covers only the inline matched name.",
"action": "Escape highlight text in MatchDetailsPanel as well, keeping only the emphasis tags, and test it.",
"lines_delta": null,
"confidence": "high"
},
{
"id": "F1.2",
"severity": "P1",
"tag": null,
"file": "src/components/map-projects/__tests__/views.test.js",
"line": 779,
"claim": "The round-trip test bypasses onSave, which still overwrites distinct highlights for rows sharing a cached target.",
"scenario": "Reopen two rows mapped to the same concept with different saved highlights, then select a row so onCSVRowSelect caches that concept by URL. During onSave, getConcept returns and mutates the same cached object for both rows. Both serialized matches consequently contain the last row's highlight. Executing the actual getConcept and save-selection code reproduced this loss; the test directly serializes restored instead.",
"action": "Build an independent concept snapshot per saved row without mutating the shared cache, and exercise the actual save path with two rows sharing a concept.",
"lines_delta": null,
"confidence": "high"
},
{
"id": "F1.3",
"severity": "P1",
"tag": null,
"file": "src/components/map-projects/MapProject.jsx",
"line": 1446,
"claim": "Restored names and highlights are still overwritten when composing the row-panel target.",
"scenario": "A Search-mapped target also appears among the row's algorithm candidates, such as a semantic candidate without highlights. Loading restores the saved highlight, but the targetConcept construction at lines 5350–5365 replaces its metadata with the candidate projection. The row panel loses the saved highlight; a candidate definition lacking names also replaces the restored localized name with the default name. Executing that construction reproduced both losses while the grid retained the restored values.",
"action": "Preserve the mapped target's restored names and highlight when merging candidate metadata into the row-panel target, and test an overlapping candidate with missing metadata.",
"lines_delta": null,
"confidence": "high"
}
]
}|
Response to Codex pass 1 F1.1 fixed in 777740f: Match Details renders highlights through the same Checked in a browser too: a reopened target that overlaps a semantic candidate with no names or highlight keeps "Hemoglobina en sangre" and "Matched name (it-IT)" in both the row panel header and the grid, before and after the row is opened. Match Details shows a highlight's other markup as text. |
paynejd
left a comment
There was a problem hiding this comment.
Codex review, pass 2 (codex-cli 0.163.0, commit 777740f)
Clean. F1.1, F1.2 and F1.3 resolved.
findings.json
{
"reviewer": "codex-cli 0.163.0",
"perspective": "defects",
"mode": "verify",
"commit": "777740f83b59d6fe6136b4827fcc55d9831e840f",
"verdict": "clean",
"prior": [
{
"id": "F1.1",
"status": "resolved"
},
{
"id": "F1.2",
"status": "resolved"
},
{
"id": "F1.3",
"status": "resolved"
}
],
"findings": []
}
paynejd
left a comment
There was a problem hiding this comment.
Simplification review (commit 777740f)
4 items, about -5 lines.
- S1 · yagni · src/components/map-projects/conceptNames.js:23 · highlightHtml's
closeparameter is never passed: both callers (getMatchedName and MatchDetailsPanel) rely on the '' default. → Make the signature(fragment, open = '<b>'), write.replaceAll('</em>', '</b>'), and drop "andclose" from the docstring. The escaping stays exactly as it is. (-1) - S2 · dup-test · src/components/map-projects/tests/views.test.js:769 ·
assert.equal(withCandidateNames(out, namedRowState, namedCache).names, searchedNames)repeats the base test 'withCandidateNames: keeps a target's own names...'. 718-7 has no candidate in namedRowState, so withCandidateNames returns the target unchanged, whichassert.equal(withCandidateNames(searched, namedRowState, namedCache), searched)already pins. → nothing replaces it (-1) - S3 · delete · src/components/map-projects/MapProject.jsx:1419 · The two-line comment above
savedMatchesrepeats withSavedMatch's JSDoc: names and highlight aren't in the CSV, and the ticket reference. → nothing replaces it; the withSavedMatch docstring on the next use already says this (-2) - S4 · shrink · src/components/map-projects/viewBuilders.js:487 · The docstring line "Returns
cachedas it is when there is none." restates thecached ? {...} : cachedternary. → Writeexport const withCachedConcept = (target, cached) => cached && {...cached, ...Object.fromEntries(...)}and drop that docstring line. (-1)
findings.json
{
"reviewer": "claude subagent",
"perspective": "simplify",
"mode": "full",
"commit": "777740f83b59d6fe6136b4827fcc55d9831e840f",
"verdict": "findings",
"prior": [],
"findings": [
{
"id": "S1",
"severity": null,
"tag": "yagni",
"file": "src/components/map-projects/conceptNames.js",
"line": 23,
"claim": "highlightHtml's `close` parameter is never passed: both callers (getMatchedName and MatchDetailsPanel) rely on the '</b>' default.",
"scenario": null,
"action": "Make the signature `(fragment, open = '<b>')`, write `.replaceAll('</em>', '</b>')`, and drop \"and `close`\" from the docstring. The escaping stays exactly as it is.",
"lines_delta": -1,
"confidence": "high"
},
{
"id": "S2",
"severity": null,
"tag": "dup-test",
"file": "src/components/map-projects/__tests__/views.test.js",
"line": 769,
"claim": "`assert.equal(withCandidateNames(out, namedRowState, namedCache).names, searchedNames)` repeats the base test 'withCandidateNames: keeps a target's own names...'. 718-7 has no candidate in namedRowState, so withCandidateNames returns the target unchanged, which `assert.equal(withCandidateNames(searched, namedRowState, namedCache), searched)` already pins.",
"scenario": null,
"action": "nothing replaces it",
"lines_delta": -1,
"confidence": "medium"
},
{
"id": "S3",
"severity": null,
"tag": "delete",
"file": "src/components/map-projects/MapProject.jsx",
"line": 1419,
"claim": "The two-line comment above `savedMatches` repeats withSavedMatch's JSDoc: names and highlight aren't in the CSV, and the ticket reference.",
"scenario": null,
"action": "nothing replaces it; the withSavedMatch docstring on the next use already says this",
"lines_delta": -2,
"confidence": "medium"
},
{
"id": "S4",
"severity": null,
"tag": "shrink",
"file": "src/components/map-projects/viewBuilders.js",
"line": 487,
"claim": "The docstring line \"Returns `cached` as it is when there is none.\" restates the `cached ? {...} : cached` ternary.",
"scenario": null,
"action": "Write `export const withCachedConcept = (target, cached) => cached && {...cached, ...Object.fromEntries(...)}` and drop that docstring line.",
"lines_delta": -1,
"confidence": "low"
}
]
}…ound unused or repeated Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… and highlights, and title the matched name with its plain text Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Adversarial security pass (fresh Claude subagent, commit 777740f) 3 findings: 2 P2, 1 P3.
Also fixed in fd87cdc: |
paynejd
left a comment
There was a problem hiding this comment.
Codex check of commits 777740f..fd87cdc (codex-cli 0.163.0, commit fd87cdc)
Clean. It checked the simplification cuts (a146c7e) and the adversarial pass's fixes (fd87cdc).
findings.json
{
"reviewer": "codex-cli 0.163.0",
"perspective": "defects",
"mode": "cuts",
"commit": "fd87cdcc3b0edc9ce9f75a50b37d7aacb8b9fca2",
"verdict": "clean",
"prior": [],
"findings": []
}
Stacked on oclmap#102 (based on its branch), so the diff below is this ticket's alone. GitHub retargets it to
mainwhen oclmap#102 merges. Merge the two together.What changed and why
A reopened project rebuilt each mapped target from its CSV: only an id, a name and a URL. oclmap#102 fills in a target's names and match highlight from the row's restored candidates, but a target mapped from the Search tab isn't among them. So its grid cell led with the repository's name, not the input-language name, and its matched name never came back.
Every save already sends each mapped target's concept, names and search highlight included, in the project's
matches, and opening a project returns it. Nothing read it back, so the first save after a reopen, autosave included, overwrote it with the stripped target.withSavedMatch,viewBuilders.js):setProjectFromDatakeys the savedmatchesbyrowIndexonce, and gives each target restored from the CSV thenamesandsearch_highlightits entry holds. An entry for another concept (another URL) is ignored. Only well-formed data comes back: names with a stringname(and a string locale and type, if any), and highlights as lists of strings.withCachedConcept):getConceptused to write each row's match into the cached concept that every row mapped to that concept shares. Rows mapped to the same concept could then show, and save, another row's match. It now returns a copy with the row's own names and match. That covers the grid cells and the save.withOwnNames) keeps the target's own names and highlight over those of a candidate for the same concept, which may have neither. So the header, the grid cell and the save agree.highlightHtml,conceptNames.js): only a highlight's<em>tags are markup, in the matched name and in Match Details. A highlight can now come from saved project data as well as from a search. The one-line matched name's tooltip uses the plain text.No change to what's saved, and none to the API. A project whose
matchesentries lack names (saved before Feb 2026, or already overwritten by a save after a reopen) opens as it does today.How I tested it
.github/workflows/pr.yml(Node 22.23.2):npm ci, eslint with--ext .jsx,.js(no problems),npm run test:all(506 unit tests and 6 integration tests pass).views.test.js, 6 tests. Two cover restoring: a target gets its saved names and highlight, and only well-formed ones. One checks that an entry for another concept, or none, leaves the target as the CSV restored it. One saves two rows mapped to one cached concept and reopens them, and each keeps its own match. Two more check thatwithCachedConceptcopies rather than writes the cache, and thatwithOwnNameskeeps a target's own over a candidate's. The two save tests fail if the cache is written instead of copied.conceptNames.test.js, 1 test: only the highlight tags are markup, in the matched name and inhighlightHtml. It fails without the escaping.Acceptance criteria
matchesentry, when that entry is for the same concept.matches.matchesentries lack them opens as it does today.Review
Not done
closes OpenConceptLab/ocl_issues#2916
🤖 Generated with Claude Code