Repository navigation
OpenConceptLab/ocl_online#660 | Hierarchy view pages children with Show more and keeps them when collapsed - #98
Conversation
… time with Show more, skeleton rows and retry, and keeps them when collapsed
snyaggarwal
left a comment
There was a problem hiding this comment.
Reviewed together with OpenConceptLab/oclapi2#988. One test coverage nit below.
| setLoadingChildren(false) | ||
| if(Array.isArray(response?.data)) { | ||
| setChildren(response.data) | ||
| setChildren(prev => page === 1 ? response.data : [...prev, ...response.data]) |
There was a problem hiding this comment.
[P4] Please add a component test for this paging state: append page 2 after page 1, fail page 2 and retry it once, then collapse and re-expand without a new request. This PR adds no automated coverage for those new paths, so the reported manual check will not catch a later duplicate-page or lost-state regression.
paynejd
left a comment
There was a problem hiding this comment.
Claude review, with a Codex second opinion (commit 4432a98)
Verdict: approve with nits. Children load 25 at a time, Show more reads num_found, a failed page shows Try again and keeps the pages already loaded, and collapse and re-expand makes no new request. I read the code and ran npm test (128 pass) and eslint on the file. No P0 or P1.
Fix before merge
Nothing blocking. Worth doing together with the paging fix on oclapi2#988:
- Append without de-duplication (P2, @snyaggarwal). Pages are joined with
[...prev, ...response.data]. If the API returns a child on two pages (the mnemonic tie on #988, or a concept added between two clicks), the child shows twice with the same React key and shares one selection. Skip children already loaded, byversion_url || url.
Not blocking
- The error row shows the server's
detailtext. For a timeout that's the English text "Narrow it with a search...", which doesn't apply inside the hierarchy and isn't translated. Always showt('common.could_not_load'). (P3; Codex rated it P2.) - The error text uses
SECONDARY_COLORS.main, which is grey.ERROR_COLORS.mainis the design system's error colour. (P3) - No automated test for the paging state (your own P4 comment). The repo's tests are plain
node --test, so a small pure function for the page merge would be testable. (P3)
Acceptance criteria
- TBv3 loads 25 children at a time, with Show more until all are shown, and an error with retry: met.
- The request matches what oclapi2#988 serves (
limit,page,num_found): met. - i18n: all three new keys are in en, es, pt-BR and zh, in the
commonsection: met. - Design system: MUI
ButtonandSkeleton, no one-off components; the error colour is the exception above.
How it was checked
Read the diff, APIService.get, the num_found usage elsewhere, the four locale files and colors.jsx. Ran the repo tests and eslint on TableResults.jsx. Did not run the UI against a large hierarchy. A Codex blind pass was clean; a Codex verification pass confirmed all four points above.
findings.json
[
{"id": "W1", "severity": "P2", "file": "src/components/search/TableResults.jsx", "line": 159, "claim": "pages appended without de-duplication", "action": "skip children already loaded", "source": "claude", "codex_check": "confirmed", "introduced_by_pr": true},
{"id": "W2", "severity": "P3", "file": "src/components/search/TableResults.jsx", "line": 166, "claim": "server detail text shown on error", "action": "always show common.could_not_load", "source": "claude", "codex_check": "confirmed", "introduced_by_pr": true},
{"id": "W3", "severity": "P3", "file": "src/components/search/TableResults.jsx", "line": 312, "claim": "error text in grey instead of ERROR_COLORS", "action": "use ERROR_COLORS.main", "source": "claude", "codex_check": "confirmed", "introduced_by_pr": true},
{"id": "W4", "severity": "P3", "file": "src/components/search/TableResults.jsx", "line": 151, "claim": "no test for paging state", "action": "extract and test the page merge", "source": "both", "codex_check": "confirmed", "introduced_by_pr": true}
]…, loaded children are de-duplicated, failed pages use the translated error, and paging state is tested
paynejd
left a comment
There was a problem hiding this comment.
Approving at 762a06f. All four points from the earlier review are fixed:
- Loaded children are de-duplicated by
version_url || url(mergeChildren), so a child returned on two pages shows once. - A failed page always shows
common.could_not_load, inERROR_COLORS.main. - The paging state is a small reducer in
childrenPaging.js, withnode --testcoverage.
Checked: npm test (135 pass) and CI's eslint command on src. A Codex check of 762a06f found nothing new, including in the next-header Show more, the hidden-row change and the map_project.try_again → common.try_again switch.
Merge after oclapi2#988, which this relies on for stable pages.
OpenConceptLab/ocl_online#660
Summary
The hierarchy view called
/children/once with no paging, so it showed only the first 25 children of any concept. It also had no way to recover from a failed load.common.show_more_of,common.could_not_load,common.try_again) in en, es, pt-BR and zh.Pairs with OpenConceptLab/oclapi2 branch
ocl_online#660, which makes/children/fast enough to page through large containers such as LOINC's parts.Testing
LOINC_PARTS_CONTAINER:npm testpasses (128 tests).🤖 Generated with Claude Code