Repository navigation
[DT-3313] [6/6] Replace the Datasets Cited filters with a column on both grids - #3937
Conversation
Coverage Report for DUOS Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
902dd9a to
af1014f
Compare
kevinmarete
changed the title
|
@fboulnois this bundles two separable changes: adding the Datasets Cited column (additive), and removing the Yes/No filter (a user-visible removal). @jlaw-codes — the removal needs your nod. Today's filter can't be a clean per-row filter: the Elasticsearch clause matches whole studies, so "Yes" returns every presentation of any study with at least one citing presentation, and the client-side pass then hides the rest. The answer depends on which siblings share the study. If that isn't a quick yes, I'll split the column out so it can merge and the removal waits. Asking before #3939 merges, since this is based on it. |
d9eced6 to
5c2b161
Compare
af1014f to
3bbe9fe
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Add citation tooltip coverage and remove stale filter entries; clean up the noted test issues.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Replaces the Presentations and Publications “Datasets Cited” filters with per-row columns and retires their URL parameters.
Changes:
- Adds reusable citation columns with Yes/No values and tooltips.
- Removes citation filters from state, UI, queries, and row filtering.
- Updates URL cleanup and related tests.
File summaries
| File | Summary |
|---|---|
test/pages/DataLibrary.spec.tsx |
Verifies citation column values. |
test/hooks/useLibraryUrlState.spec.tsx |
Tests retired URL parameter cleanup. |
test/components/data_library/LibraryFilters.spec.tsx |
Updates filter visibility expectations. |
test/components/data_library/filterRegistry.spec.ts |
Updates filter registry tests. |
test/components/data_library/columns/publicationColumns.spec.tsx |
Tests publication columns. |
test/components/data_library/columns/presentationColumns.spec.tsx |
Tests presentation columns. |
src/types/library.ts |
Removes retired filter types. |
src/libs/dataLibraryFilterConfig.ts |
Removes retired filter configuration. |
src/hooks/useLibraryUrlState.ts |
Removes and cleans retired URL parameters. |
src/components/data_library/LibraryFilters.tsx |
Removes citation filter controls. |
src/components/data_library/filterRegistry.ts |
Removes citation definitions and clauses. |
src/components/data_library/columns/sharedColumns.tsx |
Adds the shared citation column. |
src/components/data_library/columns/publicationColumns.tsx |
Adds the citation column to Publications. |
src/components/data_library/columns/presentationColumns.tsx |
Adds the citation column to Presentations. |
src/components/data_library/assets/publicationAsset.ts |
Removes publication citation filtering. |
src/components/data_library/assets/presentationAsset.ts |
Removes presentation citation filtering. |
Review details
Suppressed comments (3)
src/hooks/useLibraryUrlState.ts:85
- The retirement is incomplete:
useLibraryPageStatestill constructsavailableFilters.datasetsCitedandavailableFilters.publicationsDatasetsCitedat lines 255–262. Since that object is returned throughuseMemo, structural typing does not flag the extra properties, so dead options remain in every page state. Remove those producer entries as well.
const RETIRED_PARAMS = ['datasetsCited', 'presentationsDatasetsCited', 'publicationsDatasetsCited']
test/components/data_library/columns/presentationColumns.spec.tsx:134
- This adds a second Access-column test block; the same two assertions already exist later in this file at lines 200–209. Keeping both duplicates test maintenance and can make future changes appear covered twice. Remove one of the blocks.
describe('makePresentationColumns — Access column', () => {
test/components/data_library/columns/publicationColumns.spec.tsx:140
- This adds a second Access-column test block; the same two assertions already exist later in this file at lines 218–227. Keeping both duplicates test maintenance and can make future changes appear covered twice. Remove one of the blocks.
describe('makePublicationColumns — Access column', () => {
- Files reviewed: 16/16 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
5c2b161 to
476fc70
Compare
fdff878 to
9d9e1e2
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The citation tooltip must be made keyboard- and screen-reader-accessible.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 1
- Review effort level: Lite
261ecf1 to
28f1cb2
Compare
9d9e1e2 to
e0ff34a
Compare
4070755 to
795828a
Compare
bace108 to
fc9522c
Compare
d5b7a5a to
a92aedd
Compare
9fdb992 to
b85f04b
Compare
a92aedd to
364c713
Compare
b85f04b to
05394dc
Compare
364c713 to
50608d5
Compare
0c55403 to
20d1f56
Compare
fboulnois
left a comment
fboulnois
left a comment
There was a problem hiding this comment.
1. Citation tooltip shows on rows labelled "No": src/components/data_library/columns/sharedColumns.tsx:78
The tooltip shows a dataset citation on rows labelled "No". datasetCitation is required on the publication form (src/utils/darFormUtils.ts:396) regardless of the citation answer, so uncited publications routinely carry text; the cell reads "No" while its tooltip/accessible description names a citation.
The new spec only covers citation: false with empty text, so this combination is untested.
Fix: guard the tooltip on params.row.citation.
2. Hardcoded tabIndex={0} breaks the DataGrid roving tabindex: src/components/data_library/columns/sharedColumns.tsx:85
The hardcoded tabIndex={0} bypasses the DataGrid's roving tabindex: every visible row adds a page-level tab stop (50 extra Tab presses at pageSize 50), and Tab from outside enters the first citation cell rather than the grid's focused cell — with no focus ring, since LibraryDataGrid sets :focus-within { outline: none }.
The repo's own pattern at src/components/manage_users_table/ManageUsersTable.tsx:70-72 uses tabIndex={params.tabIndex} for exactly this reason.
Fix: use tabIndex={params.tabIndex}.
|
Both fixed in
Worth flagging on the test side: the existing |
19170c5 to
ea019e8
Compare
fdfc33a to
40f85b3
Compare
ea019e8 to
2a353c6
Compare
40f85b3 to
32a4043
Compare
2a353c6 to
73b7c77
Compare
32a4043 to
fbdcb3c
Compare
|
Presentations and Publications each offered a Yes/No "Datasets Cited" filter. A per-row fact reads better as a column than as a whole-study filter: the Elasticsearch clause matches studies, so filtering by it hid the non-citing presentations of a study that had any citing one. Both grids now carry a Datasets Cited column alongside Access. A row indexed without the field reads as "No", matching how both transforms default it, and the citation text goes in the tooltip where the index carries it. The retired params are deleted on every URL write rather than only when a filter changes, so an old link's `datasetsCited` does not survive in the URL for a user who never touches the filter panel. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Use the shared renderWithRouter helper rather than wrapping MemoryRouter inline, and assert the column's per-row values by scoping to the row's accessible role instead of filtering cells on a data-field attribute. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`useLibraryPageState` still built `datasetsCited` and `publicationsDatasetsCited` option arrays after the keys left `AvailableFilters`. The annotated `useMemo` result is not a fresh literal, so no excess-property check caught them and the dead data survived at runtime. The citation text is now the only place a citation itself is surfaced, so the tooltip carrying it is worth a test — both grids assert it appears on hover and that a row without one renders none. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The citation text was reachable only by pointer hover. The cell is now focusable, and `describeChild` makes the citation the accessible description rather than replacing Yes/No as the accessible name — the pattern datasetColumns already uses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The cited/uncited transform cases were added upstream to protect the predicate while the filter was still configured. The filter is gone here, so they go too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Presentations and Publications specs each carried the same 58 lines covering citationColumn, which is a single shared helper — Sonar flagged the duplication and it was redundant on its own terms. Its behaviour moves to sharedColumns.spec; each grid keeps a wiring assertion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
datasetCitation is required on the publication form whatever the citation answer, so uncited publications routinely carry text — a cell reading "No" described itself with a citation. Guard the tooltip on the answer. The hardcoded tabIndex also bypassed the DataGrid's roving tabindex, putting every visible row in the page tab order and taking focus from the grid's own focused cell, with no focus ring to show it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fbdcb3c to
8cf6743
Compare
Addresses
DT-3313 — 6 of 6, splitting #3926 per review. Stacked on #3939. Security risk: no.
Summary
This is the one that needs a product call, which is why it's isolated and last — it removes a filter users have today. Nothing else in the series depends on it, so a hold here blocks nothing.
Presentations and Publications each offered a Yes/No "Datasets Cited" filter. A per-row fact reads better as a column: the Elasticsearch clause matches whole studies, so selecting "Yes" kept every presentation of any study that had at least one citing presentation, including the non-citing ones. The row-level pass then hid them, which meant the filter's answer depended on which other presentations happened to share the study.
Both grids now carry a Datasets Cited column. A row indexed without the field reads as "No", matching how both transforms already default it (
citation ?? false), and the citation text goes in the tooltip where the index carries it. Because this lands after #3939, both tabs keep a filter panel throughout — they just trade this one boolean for Event/Format/Access/date or Journal/Access/date.Retired URL params (
datasetsCited,presentationsDatasetsCited,publicationsDatasetsCited) are now deleted on write. I moved that delete fromserializeFiltersToUrlintoupdateState: serialization only runs when an update carriesfilters, so an old link's param survived indefinitely for anyone who never touched the filter panel — the case the cleanup exists for. A test covers it via a query-only update.Mutation-checked: forcing the column to always read "Yes" fails 7 tests, and dropping the retired-param delete fails its own.