Implement 'Has record' filter into datahub#1329
Open
david-roper wants to merge 8 commits intoDouglasNeuroInformatics:mainfrom
Open
Implement 'Has record' filter into datahub#1329david-roper wants to merge 8 commits intoDouglasNeuroInformatics:mainfrom
david-roper wants to merge 8 commits intoDouglasNeuroInformatics:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a "Subjects with records" filter checkbox, computes scoped subject IDs with Changes
Sequence Diagram(s)sequenceDiagram
participant UI as "Filters UI\n(Checkbox)"
participant Table as "MasterDataTable\n(DataTable / Columns)"
participant Hook as "useInstrumentRecords\n(records store)"
UI->>Table: toggle "Subjects with records"
Table->>Hook: request instrument records (compute idsWithRecords)
Hook-->>Table: return idsWithRecords
Table->>Table: apply subjectId column filter { hasRecords: true/false }
Table-->>UI: reflect checkbox state
Table-->>User: render rows filtered by idsWithRecords when enabled
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/routes/_app/datahub/index.tsx`:
- Around line 337-345: idsWithRecords is undefined while the query loads and
it's built from a sliced display prefix causing false positives; eagerly
initialize a canonical lookup (e.g., const idsWithRecords = new Set<string>())
before using the query, populate that Set from full unscoped IDs returned by
useInstrumentRecords() by calling removeSubjectIdScope(record.subjectId) (do NOT
slice here), and use that Set for hasRecords checks; keep any
.slice(subjectIdDisplaySetting ?? 9) truncation only inside the cell renderer
that displays the subject ID so lookup uses the full ID and never reads .length
on undefined.
- Around line 353-363: The subject column's filter now mixes a plain string and
the HasRecordFilter shape causing filterFn (in the subject column) to receive a
string so filter.hasRecords is undefined; update all places that set the subject
filter (the code that currently writes a plain string around subjectId) to
instead store a stable object shape { query: <subjectId|string>, hasRecords:
<boolean> } (or maintain the checkbox state separately and use that when
building the filter object), and ensure the column's filterFn and any references
to HasRecordFilter (the filterFn, HasRecordFilter type usage, and the subjectId
filter setters) consistently read/write that shape so filter.hasRecords is
defined and the subject search continues to work.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7e505b7f-2d79-41b3-ac61-00140042753e
📒 Files selected for processing (1)
apps/web/src/routes/_app/datahub/index.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/web/src/routes/_app/datahub/index.tsx (3)
437-440:⚠️ Potential issue | 🔴 CriticalSearch overwrites the filter object, losing
hasRecordsstate.
setFilterValue(value)replaces the object with a string. Merge the query into the existing filter shape instead.Proposed fix
onSearchChange={(value, table) => { const subjectIdColumn = table.getColumn('subjectId')!; - subjectIdColumn.setFilterValue(value); + subjectIdColumn.setFilterValue((prev: HasRecordFilter): HasRecordFilter => ({ + ...prev, + query: value + })); }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/_app/datahub/index.tsx` around lines 437 - 440, The current onSearchChange handler calls subjectIdColumn.setFilterValue(value) which overwrites the filter object (losing hasRecords); instead read the existing filter via subjectIdColumn.getFilterValue(), merge it with the new subjectId/search value and then call subjectIdColumn.setFilterValue(mergedFilter) so existing keys like hasRecords are preserved; update the handler that references onSearchChange, table.getColumn('subjectId'), and subjectIdColumn.setFilterValue/getFilterValue to perform a shallow merge of the existing filter object with the new subjectId value before setting.
353-363:⚠️ Potential issue | 🔴 Critical
filterFndoesn't implement text search filtering.When the search box is used (line 439), the filter value becomes a string, causing
filter.hasRecordsto beundefined. The function returnstruefor all rows, effectively disabling search. ThefilterFnmust handle both thehasRecordscheckbox and the search query.Proposed fix (after extending HasRecordFilter type)
filterFn: (row, id, filter: HasRecordFilter) => { const value = row.getValue(id); if (!value) { return false; } - if (filter.hasRecords && idsWithRecords && idsWithRecords.length > 0) { - return idsWithRecords.includes(value as string); - } else { - return true; + // Check hasRecords filter + if (filter.hasRecords && idsWithRecords.size > 0) { + if (!idsWithRecords.has(value as string)) { + return false; + } } + // Check search query + if (filter.query) { + return (value as string).toLowerCase().includes(filter.query.toLowerCase()); + } + return true; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/_app/datahub/index.tsx` around lines 353 - 363, The filterFn for the column currently only checks filter.hasRecords and ignores a string search query, so when the search box supplies a string (making filter.hasRecords undefined) every row passes; update the filterFn in apps/web/src/routes/_app/datahub/index.tsx to handle both cases by checking the runtime type of filter (HasRecordFilter | string): if filter is an object with hasRecords, use idsWithRecords.includes(row.getValue(id) as string) to filter; if filter is a string, perform a text match against the cell value (row.getValue(id)) using a case-insensitive contains check; ensure you reference HasRecordFilter, filterFn, idsWithRecords and row.getValue(id) when making the change so both checkbox and search query are supported.
38-40:⚠️ Potential issue | 🟠 Major
HasRecordFiltertype needs aqueryfield to support search.The
onSearchChangehandler (line 439) sets the filter value to a plain string. This overwrites the{ hasRecords }object, causingfilter.hasRecordsto becomeundefinedin thefilterFn. Additionally, thefilterFndoesn't filter by search text.Extend the type and update all usages accordingly:
Proposed type change
type HasRecordFilter = { hasRecords: boolean; + query: string; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/_app/datahub/index.tsx` around lines 38 - 40, The HasRecordFilter type lacks a query field, so onSearchChange is overwriting the filter object with a string and breaking filterFn; update the HasRecordFilter type to include query: string, then update all places that construct or read this filter (notably the onSearchChange handler and any setFilter calls) to set/merge both hasRecords and query (e.g., setFilter(prev => ({ ...prev, query: newQuery }))), and update filterFn to check both filter.hasRecords and filter.query (perform text matching against the item when filter.query is non-empty) so filtering supports both record existence and search text.
🧹 Nitpick comments (1)
apps/web/src/routes/_app/datahub/index.tsx (1)
55-56: Variable name shadows the type.
HasRecordFilteruses PascalCase like the type. Rename tohasRecordFilterfor clarity.Proposed fix
const subjectColumn = columns.find((column) => column.id === 'subjectId')!; - const HasRecordFilter = subjectColumn.getFilterValue() as HasRecordFilter; + const hasRecordFilter = subjectColumn.getFilterValue() as HasRecordFilter;Then update line 169:
- checked={HasRecordFilter.hasRecords} + checked={hasRecordFilter.hasRecords}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/_app/datahub/index.tsx` around lines 55 - 56, The local variable name `HasRecordFilter` shadows the type name; rename the variable to `hasRecordFilter` where it's assigned from subjectColumn.getFilterValue() (currently const HasRecordFilter = subjectColumn.getFilterValue() as HasRecordFilter) and update all subsequent uses (e.g., the later reference that was suggested to change at the other location) to use `hasRecordFilter` so the type name `HasRecordFilter` remains only as the type annotation and the value variable uses camelCase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/routes/_app/datahub/index.tsx`:
- Around line 409-414: The initial filter entry with id 'subjectId' sets value
to { hasRecords: false } but HasRecordFilter now includes a query field; update
the initial filter state for the 'subjectId' entry (the object under id:
'subjectId' in apps/web/src/routes/_app/datahub/index.tsx) to include query
(e.g., query: '' or appropriate default) so the value satisfies the extended
HasRecordFilter type.
---
Duplicate comments:
In `@apps/web/src/routes/_app/datahub/index.tsx`:
- Around line 437-440: The current onSearchChange handler calls
subjectIdColumn.setFilterValue(value) which overwrites the filter object (losing
hasRecords); instead read the existing filter via
subjectIdColumn.getFilterValue(), merge it with the new subjectId/search value
and then call subjectIdColumn.setFilterValue(mergedFilter) so existing keys like
hasRecords are preserved; update the handler that references onSearchChange,
table.getColumn('subjectId'), and subjectIdColumn.setFilterValue/getFilterValue
to perform a shallow merge of the existing filter object with the new subjectId
value before setting.
- Around line 353-363: The filterFn for the column currently only checks
filter.hasRecords and ignores a string search query, so when the search box
supplies a string (making filter.hasRecords undefined) every row passes; update
the filterFn in apps/web/src/routes/_app/datahub/index.tsx to handle both cases
by checking the runtime type of filter (HasRecordFilter | string): if filter is
an object with hasRecords, use idsWithRecords.includes(row.getValue(id) as
string) to filter; if filter is a string, perform a text match against the cell
value (row.getValue(id)) using a case-insensitive contains check; ensure you
reference HasRecordFilter, filterFn, idsWithRecords and row.getValue(id) when
making the change so both checkbox and search query are supported.
- Around line 38-40: The HasRecordFilter type lacks a query field, so
onSearchChange is overwriting the filter object with a string and breaking
filterFn; update the HasRecordFilter type to include query: string, then update
all places that construct or read this filter (notably the onSearchChange
handler and any setFilter calls) to set/merge both hasRecords and query (e.g.,
setFilter(prev => ({ ...prev, query: newQuery }))), and update filterFn to check
both filter.hasRecords and filter.query (perform text matching against the item
when filter.query is non-empty) so filtering supports both record existence and
search text.
---
Nitpick comments:
In `@apps/web/src/routes/_app/datahub/index.tsx`:
- Around line 55-56: The local variable name `HasRecordFilter` shadows the type
name; rename the variable to `hasRecordFilter` where it's assigned from
subjectColumn.getFilterValue() (currently const HasRecordFilter =
subjectColumn.getFilterValue() as HasRecordFilter) and update all subsequent
uses (e.g., the later reference that was suggested to change at the other
location) to use `hasRecordFilter` so the type name `HasRecordFilter` remains
only as the type annotation and the value variable uses camelCase.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ea23aa28-a4ff-4503-be2c-a9d646cd5bc6
📒 Files selected for processing (1)
apps/web/src/routes/_app/datahub/index.tsx
joshunrau
requested changes
Apr 2, 2026
Collaborator
joshunrau
left a comment
joshunrau
left a comment
There was a problem hiding this comment.
please review the code rabbit comments
david-roper
force-pushed
the
has-record-filter
branch
from
67fb4af to
c1ded9b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/routes/_app/datahub/index.tsx (1)
437-440:⚠️ Potential issue | 🔴 CriticalSearch overwrites filter with plain string—breaks checkbox state.
setFilterValue(value)replaces the entire filter object with a string, losinghasRecords. Merge into the existing filter shape instead.Proposed fix
onSearchChange={(value, table) => { const subjectIdColumn = table.getColumn('subjectId')!; - subjectIdColumn.setFilterValue(value); + subjectIdColumn.setFilterValue((prev: HasRecordFilter): HasRecordFilter => ({ + ...prev, + query: value + })); }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/_app/datahub/index.tsx` around lines 437 - 440, The onSearchChange handler currently calls subjectIdColumn.setFilterValue(value) which replaces the filter object with a plain string and drops the existing hasRecords flag; change this to read the current filter object from subjectIdColumn.getFilterValue(), merge the new search value into that object (preserving hasRecords and any other keys), and then call subjectIdColumn.setFilterValue(mergedFilter) so the checkbox state (hasRecords) is retained; update the onSearchChange callback where subjectIdColumn and setFilterValue are used to perform this merge.
♻️ Duplicate comments (2)
apps/web/src/routes/_app/datahub/index.tsx (2)
409-414:⚠️ Potential issue | 🟠 MajorAdd
queryto initial filter state.After extending
HasRecordFilter, the initial value must includequery.,
Proposed fix
{ id: 'subjectId', value: { - hasRecords: false + hasRecords: false, + query: '' } satisfies HasRecordFilter },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/_app/datahub/index.tsx` around lines 409 - 414, The initial filter entry with id 'subjectId' uses a value typed as HasRecordFilter but is missing the required query property; update the value object in the initial filter state used by the datahub route so that the value includes query (e.g., query: '' or appropriate initial query value) alongside hasRecords, ensuring it satisfies HasRecordFilter and keeps the id 'subjectId' intact.
337-345:⚠️ Potential issue | 🟠 MajorTruncated IDs and undefined state cause incorrect filtering.
Two issues remain from prior feedback:
idsWithRecordsisundefineduntil the query resolves—iffilter.hasRecordsis toggled during loading, comparison logic may behave unexpectedly.- Slicing IDs to display length (
subjectIdDisplaySetting ?? 9) for the lookup means subjects with different full IDs but identical prefixes will incorrectly match.Initialize
idsWithRecordseagerly as an empty array/Set, and use full unscoped IDs for the lookup. Keep truncation only in the cell renderer.,
Proposed fix
const records = useInstrumentRecords(); - let idsWithRecords: string[]; - if (records.data) { - idsWithRecords = [ - ...new Set( - records.data.map((record) => removeSubjectIdScope(record.subjectId).slice(0, subjectIdDisplaySetting ?? 9)) - ) - ]; - } + const idsWithRecords = new Set<string>( + records.data?.map((record) => removeSubjectIdScope(record.subjectId)) ?? [] + );Then update the
filterFnto use full unscoped ID andSet.has():filterFn: (row, id, filter: HasRecordFilter) => { - const value = row.getValue(id); - if (!value) { - return false; - } - if (filter.hasRecords && idsWithRecords && idsWithRecords.length > 0) { - return idsWithRecords.includes(value as string); - } else { - return true; - } + const fullId = removeSubjectIdScope(row.original.id); + if (filter.hasRecords && idsWithRecords.size > 0) { + return idsWithRecords.has(fullId); + } + return true; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/_app/datahub/index.tsx` around lines 337 - 345, idsWithRecords is left undefined until useInstrumentRecords resolves and you're truncating IDs for lookup; initialize idsWithRecords eagerly (e.g., const idsWithRecords = new Set<string>() or an empty array) so toggle logic using filter.hasRecords is safe during loading, populate it when records.data exists by adding full unscoped IDs from removeSubjectIdScope(record.subjectId) (do NOT slice by subjectIdDisplaySetting), and update the filterFn to check membership with Set.has(fullUnscopedId) (or array includes) while keeping any truncation (subjectIdDisplaySetting ?? 9) only inside the cell renderer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/routes/_app/datahub/index.tsx`:
- Around line 38-40: The HasRecordFilter type currently only has hasRecords and
needs a query field to hold the search string; update the type declaration
(HasRecordFilter) to include query: string, then update any
initializations/usages (e.g., where filter state is created) so the state
includes an empty string for query, and ensure onSearchChange assigns the
incoming string to that query property; this will keep filterFn's input types
consistent with the string assignment in onSearchChange.
- Around line 352-363: The filterFn for the subject column currently treats its
filter param as HasRecordFilter but onSearchChange passes a plain string, so
searches are ignored; update the column's filter contract and implementation
(accessorFn, filterFn) to accept a unified shape or normalize the incoming
filter (e.g., if typeof filter === 'string' convert to { query: filter }) and
then apply both checks: 1) if query exists, compare it against the accessor
value (using removeSubjectIdScope(subject.id).slice(...)) to filter by search,
and 2) if hasRecords and idsWithRecords provided, ensure the value is included
in idsWithRecords; reference the existing symbols accessorFn, filterFn,
removeSubjectIdScope, subjectIdDisplaySetting, onSearchChange, and
idsWithRecords when making the change.
---
Outside diff comments:
In `@apps/web/src/routes/_app/datahub/index.tsx`:
- Around line 437-440: The onSearchChange handler currently calls
subjectIdColumn.setFilterValue(value) which replaces the filter object with a
plain string and drops the existing hasRecords flag; change this to read the
current filter object from subjectIdColumn.getFilterValue(), merge the new
search value into that object (preserving hasRecords and any other keys), and
then call subjectIdColumn.setFilterValue(mergedFilter) so the checkbox state
(hasRecords) is retained; update the onSearchChange callback where
subjectIdColumn and setFilterValue are used to perform this merge.
---
Duplicate comments:
In `@apps/web/src/routes/_app/datahub/index.tsx`:
- Around line 409-414: The initial filter entry with id 'subjectId' uses a value
typed as HasRecordFilter but is missing the required query property; update the
value object in the initial filter state used by the datahub route so that the
value includes query (e.g., query: '' or appropriate initial query value)
alongside hasRecords, ensuring it satisfies HasRecordFilter and keeps the id
'subjectId' intact.
- Around line 337-345: idsWithRecords is left undefined until
useInstrumentRecords resolves and you're truncating IDs for lookup; initialize
idsWithRecords eagerly (e.g., const idsWithRecords = new Set<string>() or an
empty array) so toggle logic using filter.hasRecords is safe during loading,
populate it when records.data exists by adding full unscoped IDs from
removeSubjectIdScope(record.subjectId) (do NOT slice by
subjectIdDisplaySetting), and update the filterFn to check membership with
Set.has(fullUnscopedId) (or array includes) while keeping any truncation
(subjectIdDisplaySetting ?? 9) only inside the cell renderer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b102a1a5-57bb-4591-be02-dff461a15c6c
📒 Files selected for processing (1)
apps/web/src/routes/_app/datahub/index.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/web/src/routes/_app/datahub/index.tsx (1)
38-40:⚠️ Potential issue | 🟠 MajorKeep
subjectIdon one filter shape.Line 440 still writes a plain string into
subjectId, but Lines 355-364 read that value asHasRecordFilter. After the first search, the subject query stops filtering rows and the checkbox state gets overwritten. Store{ query, hasRecords }end-to-end.Proposed fix
type HasRecordFilter = { hasRecords: boolean; + query: string; }; @@ filterFn: (row, id, filter: HasRecordFilter) => { - const value = row.getValue(id); - if (!value) { - return false; - } - if (filter.hasRecords) { - return idsWithRecords.has(value as string); - } + const value = (row.getValue(id) as string | undefined) ?? ''; + if (filter.query && !value.toLowerCase().includes(filter.query.toLowerCase())) { + return false; + } + if (filter.hasRecords && !idsWithRecords.has(value)) { + return false; + } return true; }, @@ id: 'subjectId', value: { - hasRecords: false + hasRecords: false, + query: '' } satisfies HasRecordFilter }, @@ onSearchChange={(value, table) => { const subjectIdColumn = table.getColumn('subjectId')!; - subjectIdColumn.setFilterValue(value); + subjectIdColumn.setFilterValue((prevValue: HasRecordFilter): HasRecordFilter => { + return { + ...prevValue, + query: value + }; + }); }}Also applies to: 355-364, 410-415, 438-440
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/routes/_app/datahub/index.tsx` around lines 38 - 40, The current filter flow stores a plain string into subjectId but parts of the code expect a HasRecordFilter shape ({ hasRecords: boolean }), causing the checkbox state to be lost; update the subject filter storage so subjectId always holds an object like { query: string, hasRecords: boolean } end-to-end: update the setter(s) that write to subjectId to store { query, hasRecords } (e.g., where setSubjectId or the subject filter value is assigned), and update readers that assume a string to read .query and .hasRecords (e.g., the components/handlers that read subjectId to build queries and the checkbox state logic), and adjust any types/interfaces so HasRecordFilter includes query: string to keep type-safety across functions that reference subjectId and HasRecordFilter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/routes/_app/datahub/index.tsx`:
- Line 25: The call to useInstrumentRecords() is missing a group scope and
therefore fetches records across all groups, causing removeSubjectIdScope() to
let unscoped subject IDs from different groups collide in the idsWithRecords
Set; update the call site (where useInstrumentRecords is invoked) to pass
groupId: currentGroup?.id so the hook only returns records for the current
group, matching the pattern used in dashboard.tsx (lines 33–38).
---
Duplicate comments:
In `@apps/web/src/routes/_app/datahub/index.tsx`:
- Around line 38-40: The current filter flow stores a plain string into
subjectId but parts of the code expect a HasRecordFilter shape ({ hasRecords:
boolean }), causing the checkbox state to be lost; update the subject filter
storage so subjectId always holds an object like { query: string, hasRecords:
boolean } end-to-end: update the setter(s) that write to subjectId to store {
query, hasRecords } (e.g., where setSubjectId or the subject filter value is
assigned), and update readers that assume a string to read .query and
.hasRecords (e.g., the components/handlers that read subjectId to build queries
and the checkbox state logic), and adjust any types/interfaces so
HasRecordFilter includes query: string to keep type-safety across functions that
reference subjectId and HasRecordFilter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9d7aefed-3d7c-4416-a3c0-0ebdf38a27b6
📒 Files selected for processing (1)
apps/web/src/routes/_app/datahub/index.tsx
There was a problem hiding this comment.
Pull request overview
Adds a “Subjects with records” filter to the Data Hub subjects table so users can restrict the list to subjects that have instrument records.
Changes:
- Introduces a new checkbox filter (“With records only”) wired into the table’s column filtering state.
- Fetches instrument records for the active group and derives a subject-id set to support the filter.
- Adjusts subject ID cell rendering to consistently truncate based on the subject ID display setting.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+338
to
+348
| const records = useInstrumentRecords({ | ||
| params: { | ||
| groupId: currentGroup?.id | ||
| } | ||
| }); | ||
| const idsWithRecords = new Set<string>(); | ||
| if (records.data) { | ||
| records.data.forEach((record) => { | ||
| idsWithRecords.add(removeSubjectIdScope(record.subjectId)); | ||
| }); | ||
| } |
There was a problem hiding this comment.
useInstrumentRecords fetches full instrument record objects for the entire group to compute idsWithRecords. If records include large payloads, this can be very expensive in the Data Hub list view and scale poorly with data volume. If possible, prefer a lightweight API/query for “subjectIds with at least one record” (or a server-side aggregated endpoint), and memoize the derived Set so it isn’t rebuilt on every render.
Contributor
There was a problem hiding this comment.
@joshunrau performance issue here which may require more extensive backend addition?
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.
Adds a filter to display subjects that have records only
closes issue #1282
Summary by CodeRabbit