[DT-3313] Add filters to Data Library asset tabs that had none or few - #3926
Open
kevinmarete wants to merge 11 commits into
Open
kevinmarete wants to merge 11 commits into
kevinmarete wants to merge 11 commits into
Conversation
Resolve conflicts with the date-range work that landed on develop: register presentationDate and publicationPublishedDate in DATE_RANGE_BOUNDS and DATE_SECTION_CONFIG so the new ranges get the same inverted-bounds handling, and gate their clauses and row predicates on isFilterActive. The auto-merge left the IP and funding row predicates returning early when their date range was inactive, which skipped the ipType/ipStatus and funderName checks added on this branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Derive each asset's filter options with that asset's own filters cleared. They were derived from a corpus already narrowed by the filter being offered, so checking one value dropped every sibling option and a multi-select checkbox group could never hold more than one value. Filters set on other tabs still apply. Seed parseFiltersFromUrl with EMPTY_FILTERS: the object is built entirely from the param configs and cast to FilterState, so a key missing from them parsed as undefined and crashed the first filters[key].length read. A round-trip test now covers every key so an unregistered one fails loudly rather than silently dropping out of the URL. Pin the study.assets.* path of every asset filter clause. A wrong path builds a valid query that matches nothing, which neither the typechecker nor a row-level test can catch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate findings affect date filtering, filter-option derivation, and citation-data access.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds configuration-driven filters, URL persistence, row filtering, and supporting columns across under-filtered Data Library tabs.
Changes:
- Adds asset-specific filters for models, workspaces, presentations, publications, intellectual property, and funding resources.
- Adds date-range handling, full-corpus filter options, and Cloud/Access columns.
- Expands test coverage and removes citation filters.
File summaries
| File | Description |
|---|---|
test/pages/DataLibrary.spec.tsx |
Updates nested presentation filtering coverage. |
test/hooks/useLibraryUrlState.spec.tsx |
Tests filter URL parsing and round-tripping. |
test/hooks/useLibraryPageState.spec.tsx |
Tests dynamic option derivation. |
test/components/data_library/LibraryFilters.spec.tsx |
Updates visible filter assertions. |
test/components/data_library/filterRegistry.spec.ts |
Tests filter definitions and query paths. |
test/components/data_library/columns/workspaceColumns.spec.tsx |
Tests the workspace Cloud column. |
test/components/data_library/columns/publicationColumns.spec.tsx |
Tests the publication Access column. |
test/components/data_library/columns/presentationColumns.spec.tsx |
Tests the presentation Access column. |
test/components/data_library/columns/modelColumns.spec.tsx |
Tests the model Cloud column. |
test/components/data_library/assets/workspaceAsset.spec.ts |
Tests workspace filtering and Cloud mapping. |
test/components/data_library/assets/publicationAsset.spec.ts |
Tests publication filters and Access mapping. |
test/components/data_library/assets/presentationAsset.spec.ts |
Tests presentation filters and Access mapping. |
test/components/data_library/assets/modelAsset.spec.ts |
Tests model filters and Cloud mapping. |
test/components/data_library/assets/intellectualPropertyAsset.spec.ts |
Tests intellectual property filters. |
test/components/data_library/assets/fundingResourceAsset.spec.ts |
Tests funder-name filtering. |
src/types/library.ts |
Adds filter state and available-filter types. |
src/libs/dataLibraryFilterConfig.ts |
Configures visible filters by asset type. |
src/hooks/useLibraryUrlState.ts |
Adds filter URL parsing and serialization. |
src/hooks/useLibraryPageState.ts |
Derives dynamic filter options. |
src/components/data_library/LibraryFilters.tsx |
Renders new filter controls. |
src/components/data_library/filterRegistry.ts |
Defines filter controls and query clauses. |
src/components/data_library/columns/workspaceColumns.tsx |
Adds the workspace Cloud column. |
src/components/data_library/columns/publicationColumns.tsx |
Adds the publication Access column. |
src/components/data_library/columns/presentationColumns.tsx |
Adds the presentation Access column. |
src/components/data_library/columns/modelColumns.tsx |
Adds the model Cloud column. |
src/components/data_library/assets/workspaceAsset.ts |
Maps and filters workspace fields. |
src/components/data_library/assets/publicationAsset.ts |
Applies publication predicates. |
src/components/data_library/assets/presentationAsset.ts |
Applies presentation predicates. |
src/components/data_library/assets/modelAsset.ts |
Maps and filters model fields. |
src/components/data_library/assets/intellectualPropertyAsset.ts |
Applies intellectual property predicates. |
src/components/data_library/assets/fundingResourceAsset.ts |
Applies funding-resource predicates. |
Review details
Suppressed comments (1)
src/libs/dataLibraryFilterConfig.ts:48
- These replacements remove the only Data Library controls for the existing
citationvalues, while the Presentation and Publication grid columns still do not render that field. After this change users can no longer inspect or filter citation data in either tab. Please retain the old filters or add a visible Citation column before dropping them.
visibleFilters: ['publicationJournal', 'publicationAccess', 'publicationPublishedDate'],
},
[AssetType.PRESENTATIONS]: {
visibleFilters: ['presentationEvent', 'presentationFormat', 'presentationAccess', 'presentationDate'],
- Files reviewed: 31/31 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.
The Cloud columns added to AI Models and Workspaces were byte-identical, as were the Access columns added to Presentations and Publications, which is most of the duplication Sonar reports on this PR. Both shapes already existed: the model/workspace Tags columns are the same chip list, and Workspaces already carried the same Access column. One chipListColumn and one truncatedTextColumn now back all seven call sites. Rendering is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Derive filter options from a corpus narrowed only by the search term. Every active filter is applied to the counts query at the Elasticsearch level and those clauses match whole studies, so clearing an asset's filters after the response came back could not recover a value whose only study the filter had already removed. When nothing is filtered this resolves to the same query key as the counts query, so it shares that request. The previous regression test could not catch this: both models lived in one study. Treat a missing date as matching neither bound in the presentation and publication predicates, mirroring the ES range clause, which never matches a document without the field, and the existing funding predicate. Intellectual Property carried the same bug from before this branch and is fixed with them, so all three read the same way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Filter-option derivation clears all active filters, so cross-tab filtering is not preserved.
Review details
Suppressed comments (1)
src/hooks/useLibraryPageState.ts:124
- This query passes
EMPTY_FILTERS, so it also removes filters owned by other tabs. For example, withaccessManagement=['controlled']active, model options are populated from studies outside the controlled scope, even though the surrounding state handling says filters combine across tabs. Build each asset's option corpus with only that asset's own filter keys cleared while retaining the other active filters (or use facet aggregations); a single all-empty corpus does not preserve the stated cross-tab behavior.
const { data: optionCorpusResponse } = useLibraryTabCounts(
libraryConfig,
EMPTY_FILTERS,
urlState.query ?? '',
)
- Files reviewed: 32/32 changed files
- Comments generated: 0 new
- Review effort level: Lite
Deriving options from an entirely unfiltered corpus dropped the filters other tabs had set, so a tab could offer values its own grid excludes. Each asset now gets a corpus with only its own filter keys cleared, via useQueries. Filter sets that come out identical share one cache entry, so this is a single request with nothing filtered — the same one the counts query makes — plus one per asset that actually owns an active filter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Two moderate findings remain in useLibraryPageState.ts regarding selected options and option-query loading/error handling.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
src/hooks/useLibraryPageState.ts:128
- The new option-corpus queries are not included in the loading state exposed by this hook.
LibraryPageShellstill passes onlyisMetadataLoadingtoLibraryFilters, so when these queries are pending after a filter/search change, the new sections can receive emptyavailableFiltersand render “No filters available” instead of a loading or previous state; query failures are indistinguishable from a corpus with no values. Expose/handle the option-query loading/error state or preserve the previous options while refetching.
const optionCorpusResults = useLibraryTabCountsFor(libraryConfig, optionCorpusFilterSets, urlState.query ?? '')
- Files reviewed: 33/33 changed files
- Comments generated: 1
- Review effort level: Lite
A value selected on the current tab can be absent from the corpus its options are derived from, because a filter owned by another tab excludes every study carrying it. The checkbox then disappeared while the filter stayed active, and the external chips deliberately skip keys the tab renders itself, so nothing was left to clear it with. Selected values are now always present in their own section's options. useQueries starts a new query key empty rather than carrying the previous answer, so a filter set on another tab blanked every section to "No filters available" for a tick. Fall back to the counts response, which is already loaded and narrower only by this asset's own filters. Toggling a filter on the tab being viewed does not change that tab's corpus key at all, so the usual interaction never refetches these in the first place. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Four moderate findings remain unresolved, covering option-corpus query fan-out and citation URL/control regressions.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
src/hooks/useLibraryTabCounts.ts:95
- This creates one full
STUDIES_AGG/top_hitsquery per distinct asset filter set. With filters persisted across several tabs, a single filter edit can fan out into up to eight option-corpus requests plus the counts request, each materializing up to 10,000 study documents; that will scale poorly for the Data Library. Prefer facet aggregations, batching, or another bounded strategy before making this the default path.
src/hooks/useLibraryUrlState.ts:296 - Removing the citation serializers also removes the only code that deleted their old URL aliases. A user opening an existing
?datasetsCited=...,?presentationsDatasetsCited=..., or?publicationsDatasetsCited=...URL and then clearing or changing filters will keep those unsupported parameters in the URL forever, becauseupdateStatecopies the current search params before serialization. Delete these legacy parameters even though they are no longer parsed.
src/hooks/useLibraryPageState.ts:128
- This starts one full
STUDIES_AGGoption-corpus query per distinct filter set. Once filters are active on several asset tabs, a single checkbox change can create up to eight newsize: 0requests in addition to the main counts request, and each aggregation returnstop_hitswithstudy.*for up to 10,000 studies. That creates an N+1/full-corpus fan-out as filters persist across tabs; please consolidate this into one facet query or lazily fetch only the option corpus needed for the current view.
const optionCorpusResults = useLibraryTabCountsFor(libraryConfig, optionCorpusFilterSets, urlState.query ?? '')
src/libs/dataLibraryFilterConfig.ts:48
- These replacements remove the existing
datasetsCited/publicationsDatasetsCitedcontrols, while the Presentation and Publication grids still expose nocitationcolumn. That makes the indexed citation state unreachable in the Data Library and is a user-visible regression; please either retain the filters or add a Citation column before removing them.
visibleFilters: ['publicationJournal', 'publicationAccess', 'publicationPublishedDate'],
},
[AssetType.PRESENTATIONS]: {
visibleFilters: ['presentationEvent', 'presentationFormat', 'presentationAccess', 'presentationDate'],
- Files reviewed: 33/33 changed files
- Comments generated: 0 new
- Review effort level: Lite
Only the visible tab renders a filter panel, so only its corpus is fetched rather than one per asset. Every other asset reads the counts response, which is already loaded; their options only label external chips, which fall back to the raw value. Clearing the tab's own keys is a no-op until it actually has a filter set, so this shares the counts query's request until then — at most one extra request, where the fan-out could previously reach eight. Serialization also deletes datasetsCited, presentationsDatasetsCited and publicationsDatasetsCited. Dropping their serializers took away the only code that removed them, and updateState serializes over a copy of the current params, so an old link's parameters would have ridden along forever. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Removing the citation filters leaves the existing citation field without a user-facing representation.
Review details
Suppressed comments (2)
src/components/data_library/filterRegistry.ts:99
- Removing the citation filter definitions also removes the only rendered path for the existing
citationdata: both asset transforms still populatecitation, but neither the Presentation nor Publication columns exposes it. Users following existing Datasets Cited links will therefore lose any way to see this field; please retain the filters or add a Citation column before removing them.
const BOOL_FILTER_KEYS: Array<'instantApproval'> = [
'instantApproval',
]
src/libs/dataLibraryFilterConfig.ts:48
- Removing these two existing filters leaves
citationwith no user-facing representation: neither the Publications nor Presentations column definitions exposes it, while both transforms still carry the field. This regresses the only way users could identify cited/non-cited assets; please retain the filters or add Citation columns before removing them.
visibleFilters: ['publicationJournal', 'publicationAccess', 'publicationPublishedDate'],
},
[AssetType.PRESENTATIONS]: {
visibleFilters: ['presentationEvent', 'presentationFormat', 'presentationAccess', 'presentationDate'],
- Files reviewed: 33/33 changed files
- Comments generated: 0 new
- Review effort level: Lite
The citation booleans were removed as filters while no grid column exposed the field, so the indexed citation state had no user-facing representation at all. Both grids now carry it, sorting and filtering on the rendered Yes/No and showing the citation text in a tooltip where the index has it. A missing field reads as No, matching how both transforms default it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The requested filters, state handling, columns, row filtering, and test coverage are complete with no unresolved blocking issues.
Review details
- Files reviewed: 33/33 changed files
- Comments generated: 0 new
- Review effort level: Lite
kevinmarete
marked this pull request as ready for review
September 14, 2026 21:47
kevinmarete
requested review from
fboulnois and
otchet-broad
and removed request for
a team
September 14, 2026 21:47
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses
DT-3313
Security risk: no
Summary
Several Data Library tabs shipped with zero or minimal filtering. AI Models had none; Intellectual Property, Workspaces, Presentations, Publications and Funding Resources had one or two each. This brings every tab up to parity with Studies/Datasets/Clinical Trials/Biospecimens using fields already in the index.
Added — AI Models: Format, License, Cloud, Tags. Intellectual Property: Type, Status. Workspaces: Cloud, Access. Presentations: Event, Format, Access, Presentation Date. Publications: Journal, Access, Published Date. Funding Resources: Funder Name. Each new filter follows the five steps in
.github/copilot-instructions.md:FilterState/AvailableFilters, clause + section behaviour infilterRegistry.ts, per-AssetTypevisibility indataLibraryFilterConfig.ts, URL parse/serialize, and tests. Cloud and Access grid columns were added so every filter has a visible counterpart.Removed — the "Datasets Cited (Presentations)?" and "Datasets Cited (Publications)?" booleans. Neither grid exposed a
citationcolumn, so dropping the filters would have left the field with no user-facing representation at all; both grids now carry a Datasets Cited column instead, sorting on the rendered Yes/No and showing the citation text in a tooltip where the index has it.Also fixed — filter option lists were derived from the active tab's current page, and from a corpus already narrowed by the filter being offered. The second half meant checking one value dropped every sibling option, so a multi-select checkbox group could never hold more than one value. Options are now derived from the full corpus with the asset's own filters cleared; filters set on other tabs still apply.
parseFiltersFromUrlbuildsFilterStateentirely from the param configs and casts, so a key missing from them parsed asundefinedand crashed the firstfilters[key].lengthread — it is now seeded fromEMPTY_FILTERS.AI Models, previously unfilterable, with two formats selected at once and the new Cloud column:
The Cloud columns added to AI Models and Workspaces were byte-identical, as were the Access columns added to Presentations and Publications. Both shapes already existed in the file they were copied into, so
chipListColumnandtruncatedTextColumnincolumns/sharedColumns.tsxnow back all seven call sites, rendering unchanged. That took Sonar's duplication on new code from 7.9% to 2.5%; what remains is thebuildQueryboilerplate every asset definition shares and the render-a-cell harness every column spec shares, both of which predate this PR and span files it does not otherwise touch.Two things a reviewer may want to look at directly.
developis merged in (57 commits); the date-range rework that landed there now covers the new Presentation Date and Published Date ranges, and resolving that merge surfaced the Intellectual Property and Funding Resources row predicates skipping their new Type/Status/Funder Name checks whenever no date range was set. Separately, every asset filter'sstudy.assets.*field path is now pinned by a test — a wrong path builds a valid query that quietly matches nothing, and neither the typechecker nor a row-level test catches it.Lint and type-check clean; full suite green (4,682 tests).
🤖 Generated with Claude Code