Repository navigation
[DT-3313] [1/6] Extract duplicated grid columns and derive the filter-panel dispatch - #3935
Merged
Merged
Conversation
Contributor
Coverage Report for DUOS Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
kevinmarete
changed the title
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The extracted tab-count query options remain module-private and cannot be reused as described.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR extracts duplicated Data Library grid columns and prepares reusable filter and tab-count query infrastructure.
Changes:
- Adds shared chip-list and truncated-text column builders.
- Updates Model and Workspace columns to use shared builders.
- Seeds URL filters with defaults.
- Extracts tab-count query options.
File summaries
| File | Description |
|---|---|
src/hooks/useLibraryUrlState.ts |
Seeds parsed filters with EMPTY_FILTERS. |
src/hooks/useLibraryTabCounts.ts |
Extracts tab-count query configuration. |
src/components/data_library/columns/workspaceColumns.tsx |
Uses shared column builders. |
src/components/data_library/columns/sharedColumns.tsx |
Adds reusable column builders. |
src/components/data_library/columns/modelColumns.tsx |
Uses shared column builders. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- 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 Models and Workspaces grids each hand-rolled the same two column shapes: a chip list capped at three with the rest behind a "+N" tooltip, and an ellipsised single-line cell with the full text in a tooltip. Presentations and Publications keep their own tag columns; theirs use `nowrap` and no overflow tooltip, so folding them in would change behaviour rather than just deduplicate. Also seeds `parseFiltersFromUrl` with `EMPTY_FILTERS`, so a key missing from every param config reads as empty rather than arriving undefined and throwing on `filters[key].length`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
kevinmarete
force-pushed
the
km-dt-3313-shared-columns
branch
from
September 15, 2026 23:52
3e28c46 to
2738b2f
Compare
LibraryFilters kept its own key lists — CHECKBOX_FILTER_KEYS, BOOLEAN_FILTER_KEYS and a hardcoded chain of date keys — duplicating what FILTER_CONTROL_BY_KEY already records. A key registered in the registry but absent from the matching list fell through to `null`, so the filter existed everywhere except the panel. Every section already carries the registry's own `control`, so dispatch reads that. The two key types come from FilterState rather than a hand list, so the narrowing cannot drift either. `participantCount` and `biospecimenPostMortemInterval` stay explicit: both are `range` with their own renderers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
kevinmarete
changed the title
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
Only a non-blocking documentation nit remains.
Review details
Suppressed comments (1)
src/components/data_library/columns/sharedColumns.tsx:10
- This helper sets
sortable: falsebelow, so the grid cannot sort these columns even though the docstring saysvalueGettermakes the text available to the grid's sorting. Please update the comment to mention only the behavior that is actually enabled (or make sortability configurable if sorting is part of the intended contract).
* A list of values as chips, collapsing everything past the third into a `+N`
* chip whose tooltip carries the rest. `valueGetter` joins them so the grid's
* own sorting and quick filter see the text, not the array.
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
kevinmarete
force-pushed
the
km-dt-3313-shared-columns
branch
from
September 16, 2026 01:55
1bdb71d to
5d5345c
Compare
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
kevinmarete
force-pushed
the
km-dt-3313-shared-columns
branch
from
September 16, 2026 02:46
5d5345c to
79478ae
Compare
|
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 — 1 of 6, splitting #3926 per review. Security risk: no.
Summary
Groundwork, no behaviour change. Two lots of duplicated knowledge removed.
Grid columns. Models and Workspaces each hand-rolled the same two column shapes: a chip list capped at three with the remainder behind a
+Ntooltip, and an ellipsised single-line cell with the full text in a tooltip. Both move tosharedColumns.tsx. Presentations and Publications keep their own tag columns — theirs useflexWrap: 'nowrap'with no overflow tooltip and novalueGetter, so folding them in would change what renders and make the column sortable.Filter-panel dispatch.
LibraryFilterskept its ownCHECKBOX_FILTER_KEYSandBOOLEAN_FILTER_KEYSlists plus a hardcoded chain of date keys, all duplicating whatFILTER_CONTROL_BY_KEYalready records. A key registered in the registry but missing from the matching list fell through tonull, so the filter existed everywhere except the panel — which is exactly what happened three times while building 3/4/5 before this landed. Every section already carries the registry'scontrol, so dispatch reads that, and the two key types derive fromFilterStaterather than a hand list so the narrowing cannot drift either.participantCountandbiospecimenPostMortemIntervalstay explicit: both arerangewith their own renderers.DATE_SECTION_CONFIGstays as it is — it carries per-filter field labels and the inverted-range message, which genuinely must be added per date filter. A date filter registered without an entry is skipped rather than throwing, and the per-tab render tests are what catch a missing one.Also seeds
parseFiltersFromUrlwithEMPTY_FILTERS: the spreads it merges areRecords theas FilterStatecast cannot check, so a key absent from every param config arrivesundefinedand throws onfilters[key].length.Net −104 lines. No test changes for the column extraction — the existing specs already cover chip counts, the
+Noverflow, empty arrays and sortability, and pass untouched. Mutation-checked: the extracted chip cap 3→2 fails 9 tests, and forcing each of the three dispatch predicates false fails 8, 9 and 2 respectively.