Repository navigation
[DT-3313] [3/6] Add filters to the AI Models and Workspaces tabs - #3938
Conversation
Coverage Report for DUOS Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
3cc2985 to
b91316e
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved issues affect filter visibility, URL persistence, and global Cloud search.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds configuration-driven filters for AI Models and Workspaces, including Cloud columns and legacy scalar-value normalization.
Changes:
- Adds filter definitions, corpus-derived options, query clauses, and row predicates.
- Adds Cloud columns and normalizes scalar-or-array fields.
- Extends types and tests for filtering, rendering, and normalization.
Outstanding review findings:
modelAsset.ts— moderate (1 vote): Include model Cloud values in global search.workspaceAsset.ts— moderate (1 vote): Include workspace Cloud values in global search.filterRegistry.ts— moderate (1 vote): Register all six filters for URL parsing and serialization.filterRegistry.ts— nit (2 votes): Add query-clause path and multi-value OR assertions.dataLibraryFilterConfig.ts— moderate (2 votes): Add the six filters toCHECKBOX_FILTER_KEYSand cover panel rendering.library.ts— moderate (2 votes): Add the six filters toARRAY_FILTER_PARAM_CONFIGwith round-trip coverage.
File summaries
| File | Reviewed changes |
|---|---|
test/components/data_library/LibraryFilters.spec.tsx |
Updates filter fixtures. |
test/components/data_library/filterRegistry.spec.ts |
Tests filter registration. |
test/components/data_library/columns/workspaceColumns.spec.tsx |
Tests workspace Cloud rendering. |
test/components/data_library/columns/modelColumns.spec.tsx |
Tests model Cloud rendering. |
test/components/data_library/assets/workspaceAsset.spec.ts |
Tests workspace normalization and predicates. |
test/components/data_library/assets/modelAsset.spec.ts |
Tests model normalization and predicates. |
src/types/library.ts |
Adds model and workspace filter state types. |
src/types/elastic.ts |
Supports scalar or array Cloud values. |
src/libs/dataLibraryFilterConfig.ts |
Registers visible filters. |
src/hooks/useLibraryPageState.ts |
Derives full-corpus filter options. |
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/modelColumns.tsx |
Adds the model Cloud column. |
src/components/data_library/assets/workspaceAsset.ts |
Normalizes and filters workspace rows. |
src/components/data_library/assets/modelAsset.ts |
Normalizes and filters model rows. |
src/components/data_library/assets/definition.ts |
Adds shared string-array normalization. |
Review details
Suppressed comments (3)
src/components/data_library/assets/modelAsset.ts:91
- The new Cloud column is not represented in this asset's
searchFields, which are folded intoALL_SEARCH_FIELDSfor the library's global search. Searching for a provider such as AWS or GCP therefore cannot match a model through its Cloud value; addstudy.assets.models.cloudto the search fields as well as the filter/column integration.
cloud: toStringArray(model.cloud),
src/components/data_library/assets/workspaceAsset.ts:101
- The new Cloud column is not represented in this asset's
searchFields, which are folded intoALL_SEARCH_FIELDSfor the library's global search. Searching for a provider such as AWS or GCP therefore cannot match a workspace through its Cloud value; addstudy.assets.workspaces.cloudto the search fields as well as the filter/column integration.
cloud: toStringArray(workspace.cloud),
src/components/data_library/filterRegistry.ts:50
- The URL-state parameter registry in
src/hooks/useLibraryUrlState.tsstill omits all six new array keys from both parsing and serialization. Selecting any of these filters is therefore lost when the URL state re-parses, and a URL cannot restore one; add entries for these keys and a round-trip test.
'modelFormat',
'modelLicense',
'modelCloud',
'modelTags',
'workspaceCloud',
'workspaceAccess',
- 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.
8ac602d to
c045362
Compare
b91316e to
bfac3ec
Compare
There was a problem hiding this comment.
🟢 Approval recommended
Only minor test documentation and coverage nits remain.
Review details
Suppressed comments (2)
test/hooks/useLibraryUrlState.spec.tsx:160
- This comment is now inverted: all six keys are present in
ARRAY_FILTER_PARAM_CONFIG, so they do parse and serialize. Please update the explanation to describe the regression these tests guard against; as written it tells future maintainers that the implementation is still broken.
// Registered in FilterState but absent from ARRAY_FILTER_PARAM_CONFIG, these
// parse to nothing and never serialize, so a selection dies on reload.
test/hooks/useLibraryUrlState.spec.tsx:168
- This test is named “round-trips” but only seeds the URL and asserts parsed state; it never calls
updateStateor inspectslocation.search, so a missing serializer entry would still pass all six cases. Add a serializer assertion for these keys (or split parsing and serialization cases) so the new URL mapping is covered in both directions.
])('round-trips %s through the URL', (key, value) => {
- Files reviewed: 19/19 changed files
- Comments generated: 0 new
- Review effort level: Lite
bbb9e93 to
1bf6217
Compare
2736cdb to
40a2720
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Moderate findings remain for Cloud free-text search coverage and URL serialization test coverage.
Review details
Suppressed comments (7)
src/components/data_library/assets/modelAsset.ts:24
- The new Cloud filter/column is not included in
modelAsset.searchFields, so the library's free-text search (which uses the union inALL_SEARCH_FIELDS) cannot find a model whose only matching value is its cloud provider. Addstudy.assets.models.cloudto the model search fields and cover it in the corresponding search-fields test.
if (filters.modelCloud.length > 0 && !(model.cloud || []).some(cloud => filters.modelCloud.includes(cloud))) {
src/components/data_library/assets/workspaceAsset.ts:36
- The new Cloud filter/column is not included in
workspaceAsset.searchFields, so the library's free-text search (which uses the union inALL_SEARCH_FIELDS) cannot find a workspace whose only matching value is its cloud provider. Addstudy.assets.workspaces.cloudto the workspace search fields and cover it in the corresponding search-fields test.
if (filters.workspaceCloud.length > 0 && !(workspace.cloud || []).some(cloud => filters.workspaceCloud.includes(cloud))) {
src/components/data_library/columns/modelColumns.tsx:48
- The new Cloud column is not included in
modelAsset.searchFields.ALL_SEARCH_FIELDSis built from these arrays and reused for the globalmulti_match, so searching for a provider such asAWSwill not find a model whose only matching field is Cloud, unlike the other model fields exposed here. Addstudy.assets.models.cloudto the model search fields.
chipListColumn<ModelAsset>('cloud', 'Cloud', row => row.cloud || [], 130),
src/components/data_library/columns/workspaceColumns.tsx:91
- The new Cloud column is not included in
workspaceAsset.searchFields. BecauseALL_SEARCH_FIELDSis assembled from those arrays and drives the globalmulti_match, a search forAWS/GCPwill not find a workspace whose only matching field is Cloud, even though the other displayed workspace fields are searchable. Addstudy.assets.workspaces.cloudto the workspace search fields.
chipListColumn<WorkspaceAsset>('cloud', 'Cloud', row => row.cloud || [], 130),
test/components/data_library/LibraryFilters.spec.tsx:632
- This test comment refers to a
CHECKBOX_FILTER_KEYSlist that does not exist;LibraryFiltersnow derives checkbox keys fromFilterStateand checkssection.control. The stale explanation can mislead maintainers about where a new filter must be registered.
// A key registered as a checkbox in filterRegistry still renders nothing unless
// LibraryFilters recognises it in CHECKBOX_FILTER_KEYS, so assert on the panel
// rather than on getFilterSectionsForAsset's output.
test/hooks/useLibraryUrlState.spec.tsx:168
- This test is named as a URL round-trip but only parses an initial URL; it never calls
updateStateand checks the serialized search params. A regression inserializeArrayFiltersToUrlfor any of these new keys would still pass, so exercise an update and assert the resulting URL as well.
])('round-trips %s through the URL', (key, value) => {
test/hooks/useLibraryUrlState.spec.tsx:160
- This explanatory comment is now backwards: all six keys are present in
ARRAY_FILTER_PARAM_CONFIGimmediately above the test, and that is why the test succeeds. Update the comment to describe the URL round-trip regression being covered rather than claiming the keys are absent.
// Registered in FilterState but absent from ARRAY_FILTER_PARAM_CONFIG, these
// parse to nothing and never serialize, so a selection dies on reload.
- Files reviewed: 18/18 changed files
- Comments generated: 0 new
- Review effort level: Lite
40a2720 to
4b01393
Compare
f91e841 to
bd9f2bb
Compare
996d185 to
b60e18b
Compare
fe3f3e3 to
d518a95
Compare
00e0d51 to
e249c6d
Compare
6089499 to
f2c1770
Compare
e249c6d to
0ab07cc
Compare
0ab07cc to
fffe7fd
Compare
fffe7fd to
1b19568
Compare
fboulnois
left a comment
There was a problem hiding this comment.
A few suggestions identified with Claude:
1. New model filters use case-sensitive exact matching on free-text fields: src/components/data_library/assets/modelAsset.ts:14
The new model filters (modelFormat, modelLicense, modelCloud, modelTags) re-check rows with exact, case-sensitive includes, but all four are free-text fields — AiModelAddEdit renders format/license as plain text inputs, and tags/cloud as creatable selects.
Consequence: "PyTorch" and "pytorch" become two separate checkboxes; selecting one drops the other's rows, even though the analyzed match_phrase clause already admitted both studies.
Every other free-text/corpus-derived filter in the repo uses includesIgnoreCase (workspacePlatform, clinicalTrialRegistry, clinicalTrialInterventionType, biospecimenDataUse); only enum-backed keys use exact includes.
Fix: use includesIgnoreCase for these four keys.
2. workspaceAccess uses exact matching on an unvalidated free-text field: src/components/data_library/assets/workspaceAsset.ts:40
Same class as above, but sharper: workspaceAccess uses exact includes on the line immediately after workspacePlatform uses includesIgnoreCase. access is a bare FormField with no options and no validator at all — the most variance-prone field of the set.
Fix: use includesIgnoreCase here too.
3. modelCloud and workspaceCloud share the label 'Cloud': src/components/data_library/filterRegistry.ts:534
Both keys label as 'Cloud'. On the Workspaces tab, a carried-over model filter renders as "Cloud: AWS" under "Filters from other views", visually identical to the tab's own unchecked "Cloud" section.
The repo already disambiguates its one other label collision ('Datasets Cited (Presentations)?' vs '(Publications)?').
Fix: disambiguate one of the two labels.
|
All three fixed in 1 & 2. The same collision class turned up twice more downstream — |
Models gains Format, License, Cloud and Tags; Workspaces gains Cloud and Access alongside the Tools and Platform it already had. Both grids gain a Cloud column so the new filter has something visible to act on. The Elasticsearch clauses only decide which studies enter the shared aggregation, so each row is re-checked client-side — otherwise a study with one matching model would contribute all of its models to the grid and to the tab-count badge derived from the same function. `cloud`, `tags`, `tools` and `trainedOnDatasets` are normalized to arrays. Studies indexed before Cloud became a creatable multi-select carry a bare string, and the chip column's `.join` threw on those, blanking the whole grid. The raw ES types now say `cloud?: string[] | string` so the normalization cannot be skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both were registered in filterRegistry but missing from the two lists that actually wire a filter up, so they were dead on arrival: - `CHECKBOX_FILTER_KEYS` in LibraryFilters gates rendering, and an unrecognized key falls through to `null` — none of the six sections appeared in the panel. - `ARRAY_FILTER_PARAM_CONFIG` in useLibraryUrlState drives both parse and serialize, so a selection never reached the URL and died on reload. The existing tests asserted `getFilterSectionsForAsset`'s output, which was correct throughout, and so passed either way. They now assert the rendered panel and a URL round-trip instead, plus the Elasticsearch field path each clause targets — a wrong `study.assets.*` path type-checks and silently matches nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Option lists are built from trimmed values while the new predicates compared raw ones, so selecting an option built from `" ONNX "` dropped the only row that produced it. The transforms now trim, which keeps rows, options and the grid on the same values; the older tools/platform filters never hit this because `includesIgnoreCase` does a substring match. Cloud was displayed and filterable but absent from `searchFields`, so a free-text search for a provider could not find a model or workspace whose only matching field was Cloud. The URL test claimed a round trip while only parsing, and its comment claimed the keys were unregistered; it now asserts parse, serialize and clear. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
format, license, cloud, tags and access are all free text — plain inputs or creatable selects — so the corpus can carry "PyTorch" and "pytorch" as two options. The analyzed match_phrase clause admits both studies, but the row pass used exact `includes`, so checking one dropped the other's rows. Use `includesIgnoreCase`, as every other free-text filter here already does. Disambiguate the two 'Cloud' labels while they are in reach: a carried-over model filter rendered as a chip identical to the Workspaces tab's own section. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ec18dc3 to
25a6e02
Compare
|



Addresses
DT-3313 — 3 of 6, splitting #3926 per review. Stacked on #3936. Security risk: no.
Summary
Mechanical, following the pattern #3936 establishes. AI Models gains Format, License, Cloud and Tags; Workspaces gains Cloud and Access alongside the Tools and Platform it already had. Both grids gain a Cloud column, so the new filter acts on something the user can see.
Each new filter is a checkbox group whose options come from the full corpus (#3936), a
matchAnyclause, and a client-side row predicate. The predicate is not redundant: the Elasticsearch clauses only decide which studies enter the shared aggregation, so without it a study with one matching model contributes all of its models to the grid — and to the tab-count badge, which is derived from the same function.Also carries the fix for the blank page reported on #3926. Studies indexed before Cloud became a creatable multi-select hold it as a bare string rather than an array, and the new chip column called
.joinon it, which threw and blanked the whole grid — only these two tabs, since they're the two with a Cloud column.cloud,tags,toolsandtrainedOnDatasetsare now normalized throughtoStringArray, and the raw ES types saycloud?: string[] | stringso the normalization can't be skipped. Verified against the dev corpus.Mutation-checked: short-circuiting the
modelCloudpredicate fails 2 tests, and makingtoStringArraydrop the bare-string case — i.e. reintroducing the blank-page bug — fails 4.