Improve datahub search #1252
Improve datahub search #1252
Conversation
WalkthroughAdds a header SearchBar and client-side subject filtering on the Datahub page; uses filtered subjects for CSV/Excel/JSON exports with non-empty validation and improved notifications; moves subject lookup trigger to a header button and opens IdentificationForm in a dialog; subject navigation now targets .//table. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Header as Header\n(SearchBar)
participant LookupBtn as Subject\nLookup Button
participant Dialog as Subject\nLookup Dialog
participant Table as MasterDataTable
participant Exporter as Export\nHandlers
User->>Header: type searchString
Header->>Table: update tableData (filter subjects)
User->>LookupBtn: click
LookupBtn->>Dialog: open IdentificationForm
Dialog->>Table: select subject -> navigate to ./<id>/table
User->>Exporter: request export
Exporter->>Table: read tableData (listedSubjects)
Exporter->>User: show info notification -> deliver file or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code graph analysis (1)apps/web/src/routes/_app/datahub/index.tsx (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (8)
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/web/src/routes/_app/datahub/index.tsx (1)
104-104: Avoid explicitanytype.The
: anyreturn type annotation suppresses type checking. Remove it or use a proper type.getExportRecords() - .then((data): any => { + .then((data) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/routes/_app/datahub/index.tsx(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/routes/_app/datahub/index.tsx (3)
packages/schemas/src/subject/subject.ts (1)
Subject(19-19)vendor/zod@3.x/src/v3.d.ts (1)
record(2499-2499)apps/web/src/utils/excel.ts (1)
downloadExcel(4-8)
🔇 Additional comments (4)
apps/web/src/routes/_app/datahub/index.tsx (4)
1-25: Imports look good.All new imports (
useEffect,Button,UserSearchIcon) are used in the component.
82-83: State initialization relies on useEffect sync.The
tableDatainitialization fromdataworks because theuseEffect(lines 150-164) re-syncs whendatachanges. This is acceptable but consider using a derived value pattern if React 19's features allow.
150-164: Filtering logic is sound.The
useEffectcorrectly syncstableDatawithdataand applies the search filter. Dependencies are complete.
219-223: Filtered data correctly passed to table.Using
tableDataensures the table reflects the search filter.Note: Row selection navigates to
./table(line 222) while subject lookup navigates to./assignments(line 146). Verify this is intentional.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/src/routes/_app/datahub/index.tsx (1)
188-199: Clean up Dialog trigger Button classes and IDs for clarity.The trigger Button still carries
[&>input]-scoped classes from the old SearchBar and uses “search-bar” in its id/testid, which no longer reflects its role as a lookup trigger. You can simplify the styles and make the identifiers clearer:- <Dialog open={isLookupOpen} onOpenChange={setIsLookupOpen}> - <Dialog.Trigger> - <Button - className="[&>input]:text-foreground [&>input]:placeholder-foreground grow gap-1" - data-testid="datahub-subject-lookup-search" - id="subject-lookup-search-bar" - > + <Dialog open={isLookupOpen} onOpenChange={setIsLookupOpen}> + <Dialog.Trigger> + <Button + className="grow gap-1" + data-testid="datahub-subject-lookup-button" + id="subject-lookup-button" + > <UserSearchIcon />{' '} {t({ en: 'Subject Lookup', fr: 'Trouver un client' })} </Button> </Dialog.Trigger>This removes dead CSS selectors and avoids conflating the trigger with a search input.
🧹 Nitpick comments (2)
apps/web/src/routes/_app/datahub/index.tsx (2)
40-41: ReuseremoveSubjectIdScopeinstead of duplicating regex logic.You already import and use
removeSubjectIdScopefor display, but export/filter paths still hand-roll the same normalization via regex. Centralizing this reduces drift and keeps behavior consistent across UI, filtering, and exports.- const listedSubjects = tableData.map((record) => { - return record.id.replace(/^.*?\$/, ''); - }); + const listedSubjects = tableData.map((record) => removeSubjectIdScope(record.id));- const filtered = data.filter((record) => - record.id - .replace(/^.*?\$/, '') - .toLowerCase() - .includes(searchString.toLowerCase()) - ); + const filtered = data.filter((record) => + removeSubjectIdScope(record.id) + .toLowerCase() + .includes(searchString.toLowerCase()) + );Please double-check that
removeSubjectIdScope’s behavior matches the regex assumption (strip everything up to the first$).Also applies to: 105-107, 156-160
94-120: Avoidanyin the export pipeline to keepInstrumentRecordsExporttyped.
getExportRecordsis already typed to returnInstrumentRecordsExport, so explicitly annotating the.thenparameter asanydrops useful type checking forsubjectIdand friends. Let TypeScript infer the type (or make it explicit) instead:- getExportRecords() - .then((data): any => { + getExportRecords() + .then((data) => { const listedSubjects = tableData.map((record) => { return record.id.replace(/^.*?\$/, ''); }); const filteredData = data.filter((dataEntry) => listedSubjects.includes(dataEntry.subjectId)); switch (option) { case 'CSV': void download('README.txt', t('datahub.index.table.exportHelpText')); void download(`${baseFilename}.csv`, unparse(filteredData)); break; case 'Excel': return downloadExcel(`${baseFilename}.xlsx`, filteredData); case 'JSON': return download(`${baseFilename}.json`, JSON.stringify(filteredData, null, 2)); } })This keeps the nice “export only what’s currently listed” behavior while preserving static safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/routes/_app/datahub/index.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/routes/_app/datahub/index.tsx (2)
packages/schemas/src/subject/subject.ts (1)
Subject(19-19)apps/web/src/utils/excel.ts (1)
downloadExcel(4-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-and-test
🔇 Additional comments (2)
apps/web/src/routes/_app/datahub/index.tsx (2)
81-84: Ensuredatais always an array before using it in state/effect.Both
useState<Subject[]>(data)anddata.filter(...)assumedatais aSubject[]. IfuseSubjectsQueryever returnsundefinedduring loading or error,MasterDataTablecan receiveundefinedand the effect will throw ondata.filter. Safer to normalize to an empty array once and reuse that in state and filtering:- const { data } = useSubjectsQuery({ params: { groupId: currentGroup?.id } }); - const [tableData, setTableData] = useState<Subject[]>(data); + const { data } = useSubjectsQuery({ params: { groupId: currentGroup?.id } }); + const subjects = data ?? []; + const [tableData, setTableData] = useState<Subject[]>(subjects); useEffect(() => { if (!searchString) { - setTableData(data); + setTableData(subjects); return; } - const filtered = data.filter((record) => + const filtered = subjects.filter((record) => record.id .replace(/^.*?\$/, '') .toLowerCase() .includes(searchString.toLowerCase()) ); setTableData(filtered); - }, [searchString, data]); + }, [searchString, data]);Also applies to: lines 150-165
137-147: Verify navigation targets: lookup goes to/assignments, table rows to/table.
lookupSubjectnavigates to./${response.data.id}/assignments, while row selection inMasterDataTablenow goes to./${subject.id}/table. If both flows are meant to land users on the same "subject view", this divergence may be confusing. If the intent is for both to open the new table view, update the lookup navigation accordingly; if the difference is intentional, consider documenting it or naming the actions to make the distinction clear.
There was a problem hiding this comment.
Actionable comments posted: 0
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)
137-147: Align subject navigation between lookup and table selection.
onSelectnow navigates to./${subject.id}/table, butlookupSubjectstill sends users to./${response.data.id}/assignments. For a consistent UX (and to match the walkthrough steps that point at/datahub/123/table), consider routing the lookup flow to the table view as well:- await navigate({ to: `./${response.data.id}/assignments` }); + await navigate({ to: `./${response.data.id}/table` });Unless there’s a deliberate reason to land on Assignments here, this change would make the two entry points behave consistently.
Also applies to: 224-225
🧹 Nitpick comments (3)
apps/web/src/providers/WalkthroughProvider.tsx (1)
140-176: Walkthrough targeting matches new UI; minor French copy fix.The updated targets
#datahub-subject-search-barand[data-spotlight-type="subject-lookup-search-button"]correctly match the new Datahub search bar and lookup button. One small nit: in the French text for the Subject Lookup step,"cliquez sur la button de recherche"should be"cliquez sur le bouton de recherche"for correct grammar.apps/web/src/routes/_app/datahub/index.tsx (2)
81-84: Harden subject filtering state and reuse ID normalization logic.The new
tableData/searchStringflow and tying exports totableDatalook good, but a few small tweaks would make this more robust and maintainable:
- If
useSubjectsQuerycan ever yieldundefinedduring loading,useState<Subject[]>(data)andsetTableData(data)will temporarily pushundefinedintotableData, which could break.map/ClientTable. To be defensive, defaultdatato an empty array and use that consistently in the effect:- const { data } = useSubjectsQuery({ params: { groupId: currentGroup?.id } }); - const [tableData, setTableData] = useState<Subject[]>(data); + const { data = [] } = useSubjectsQuery({ params: { groupId: currentGroup?.id } }); + const [tableData, setTableData] = useState<Subject[]>(data);
- You’re duplicating the prefix-stripping regex in both the export path and the search filter. Since
removeSubjectIdScopeis already imported, using it here will keep ID normalization in one place:- const listedSubjects = tableData.map((record) => { - return record.id.replace(/^.*?\$/, ''); - }); + const listedSubjects = tableData.map((record) => removeSubjectIdScope(record.id));- const filtered = data.filter((record) => - record.id - .replace(/^.*?\$/, '') - .toLowerCase() - .includes(searchString.toLowerCase()) - ); + const filtered = data.filter((record) => + removeSubjectIdScope(record.id).toLowerCase().includes(searchString.toLowerCase()) + );
- For large datasets,
listedSubjects.includes(dataEntry.subjectId)is O(n·m). Consider using aSetto make the export filter O(n + m):- const listedSubjects = tableData.map((record) => removeSubjectIdScope(record.id)); - - const filteredData = data.filter((dataEntry) => listedSubjects.includes(dataEntry.subjectId)); + const listedSubjects = new Set( + tableData.map((record) => removeSubjectIdScope(record.id)) + ); + + const filteredData = data.filter((dataEntry) => + listedSubjects.has(dataEntry.subjectId) + );Also applies to: 105-110, 150-164, 222-222
175-201: Clean up lookup button classes; SearchBar wiring looks good.The header
SearchBarcorrectly controlssearchStringand ties into the filtering effect, and the new lookup trigger Button is wired properly to the dialog and walkthrough attributes.The Button’s
classNamestill contains[&>input]:…utilities that only applied when this was aSearchBar:<Button className="[&>input]:text-foreground [&>input]:placeholder-foreground gap-1" … >Since a Button has no child
<input>, these selectors are now dead and can be dropped for clarity:- <Button - className="[&>input]:text-foreground [&>input]:placeholder-foreground gap-1" + <Button + className="gap-1" data-spotlight-type="subject-lookup-search-button" data-testid="subject-lookup-search-button" id="subject-lookup-search-button" variant="outline" >Optionally, you might also update the search bar placeholder from “Click to Search” to something like “Type to search” now that it’s directly editable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/src/providers/WalkthroughProvider.tsx(1 hunks)apps/web/src/routes/_app/datahub/index.tsx(6 hunks)apps/web/src/styles.css(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/routes/_app/datahub/index.tsx (2)
packages/schemas/src/subject/subject.ts (1)
Subject(19-19)apps/web/src/utils/excel.ts (1)
downloadExcel(4-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-and-test
🔇 Additional comments (1)
apps/web/src/styles.css (1)
11-13: Selector rename aligns with new search bar ID.The spotlight selector now correctly targets
#datahub-subject-search-bar, matching the updated component and walkthrough usage. No issues here.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/src/routes/_app/datahub/index.tsx (1)
188-201: Clean up input-specific Tailwind selectors on the Dialog trigger ButtonThe trigger
Buttonstill has[&>input]:…selectors, which were useful for the oldSearchBarbut are now dead weight on a plain button.You can simplify the class for clarity:
- <Button - className="[&>input]:text-foreground [&>input]:placeholder-foreground gap-1" + <Button + className="gap-1" data-spotlight-type="subject-lookup-search-button" data-testid="subject-lookup-search-button" id="subject-lookup-search-button" variant="outline" >IDs and test IDs are now unique relative to the header search, which fixes the earlier duplication issue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/routes/_app/datahub/index.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/routes/_app/datahub/index.tsx (2)
packages/schemas/src/subject/subject.ts (1)
Subject(19-19)apps/web/src/utils/excel.ts (1)
downloadExcel(4-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-and-test
🔇 Additional comments (3)
apps/web/src/routes/_app/datahub/index.tsx (3)
1-1: Imports updated appropriatelyThe additional React hooks, UI components, and
UserSearchIconmatch the new stateful search UI and subject lookup button usage; no issues here.Also applies to: 4-11, 18-18
146-147: Navigation path change to./${id}/tablelooks consistentBoth the lookup flow and table row selection now navigate to
./${id}/table, which keeps subject routing consistent with the new design.Also applies to: 223-225
175-186: Header subject SearchBar wiring looks goodBinding the header
SearchBartosearchStringwithonValueChangedrives the client-side filtering cleanly;data-testid/idare unique and match the new walkthrough/spotlight usage.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/web/src/routes/_app/datahub/index.tsx (2)
81-82: Guard against undefineddata— unresolved from past review.This issue was previously flagged but not addressed. When
useSubjectsQueryis loading,dataisundefined, causingtableDatato beundefinedand breaking the export handler's.map()call at line 97.Apply this diff:
- const [tableData, setTableData] = useState<Subject[]>(data); + const [tableData, setTableData] = useState<Subject[]>(data ?? []);
163-174: Guard against undefineddatain filter effect.When
dataisundefined, line 169'sdata.filter()will throw. This is related to the initialization issue above.Apply this diff:
useEffect(() => { + const source = data ?? []; + if (!searchString) { - setTableData(data); + setTableData(source); return; } - const filtered = data.filter((record) => + const filtered = source.filter((record) => removeSubjectIdScope(record.id).toLowerCase().includes(searchString.toLowerCase()) ); setTableData(filtered); }, [searchString, data]);
🧹 Nitpick comments (1)
apps/web/src/routes/_app/datahub/index.tsx (1)
200-200: Remove unused input-targeting styles from Button.The
[&>input]:text-foreground [&>input]:placeholder-foregroundselectors target input children, but this Button has none. Leftover from the SearchBar styling.Apply this diff:
<Button - className="[&>input]:text-foreground [&>input]:placeholder-foreground gap-1" + className="gap-1" data-spotlight-type="subject-lookup-search-button"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/routes/_app/datahub/index.tsx(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-and-test
🔇 Additional comments (3)
apps/web/src/routes/_app/datahub/index.tsx (3)
97-107: Export filtering logic is correct.The consistent use of
removeSubjectIdScopeand the non-empty validation check are good improvements from the past review.
159-159: Navigation target updated correctly.The change to navigate to the table page aligns with the PR objectives.
232-234: Filtered table data and navigation are correct.Using
tableDatafor the filtered view and navigating to the table page both align with the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/src/routes/_app/datahub/index.tsx (1)
81-83: Guard search filtering againstdatabeing undefinedThe new client‑side filtering is almost there, but there’s still a crash path if the user types into the search bar before
useSubjectsQueryhas resolved:
- Line 164 safely derives
definedTableData = data ?? [].- Line 171 then calls
data.filter(...)directly. Ifdatais stillundefinedandsearchStringis non‑empty, this becomesundefined.filter(...)and throws.You can fix this by using a single local
sourcearray derived fromdatafor both the “no search” and “filtered” branches:- useEffect(() => { - const definedTableData = data ?? []; - - if (!searchString) { - setTableData(definedTableData); - return; - } - - const filtered = data.filter((record) => - removeSubjectIdScope(record.id).toLowerCase().includes(searchString.toLowerCase()) - ); - - setTableData(filtered); - }, [searchString, data]); + useEffect(() => { + const source = data ?? []; + + if (!searchString) { + setTableData(source); + return; + } + + const filtered = source.filter((record) => + removeSubjectIdScope(record.id).toLowerCase().includes(searchString.toLowerCase()) + ); + + setTableData(filtered); + }, [searchString, data]);This keeps
tableDatain sync with the query results while avoiding any undefined access.Also applies to: 163-176
🧹 Nitpick comments (1)
apps/web/src/routes/_app/datahub/index.tsx (1)
80-83: Export filtering and notifications look good; sanitize filenames for cross‑platform safetyFiltering export records by the currently listed subjects (via
tableDataandremoveSubjectIdScope) and failing fast when there are no entries to export is solid, and the pre‑export info + specific error messaging will give users clear feedback.One improvement:
baseFilenameusesnew Date().toISOString(), which includes:characters that are invalid in filenames on some platforms and can cause odd browser sanitization. Consider normalizing the timestamp before composing the filename:- const handleExportSelection = (option: 'CSV' | 'Excel' | 'JSON') => { - const baseFilename = `${currentUser!.username}_${new Date().toISOString()}`; + const handleExportSelection = (option: 'CSV' | 'Excel' | 'JSON') => { + const safeTimestamp = new Date().toISOString().replace(/[:]/g, '-'); + const baseFilename = `${currentUser!.username}_${safeTimestamp}`;Also applies to: 93-107, 109-116, 120-125, 128-147
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/routes/_app/datahub/index.tsx(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/src/routes/_app/datahub/index.tsx (3)
packages/schemas/src/subject/subject.ts (1)
Subject(19-19)packages/subject-utils/src/index.ts (1)
removeSubjectIdScope(34-39)apps/web/src/utils/excel.ts (1)
downloadExcel(4-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-and-test
🔇 Additional comments (2)
apps/web/src/routes/_app/datahub/index.tsx (2)
1-11: Imports match usage and keep concerns separatedReact hooks, libui components (including SearchBar and Button), and UserSearchIcon are all used correctly and the imports are cleanly grouped. No issues here.
Also applies to: 18-18
150-160: Subject search, lookup, and navigation wiring align with PR goals
- Header
SearchBarcorrectly drivessearchString, which feeds the filteredtableDataand therefore theMasterDataTable.- The new subject lookup
Dialog.Triggerbutton has uniqueid/data-testidand usesUserSearchIcon, avoiding the previous duplicate‑ID issue.- Both subject lookup success (
lookupSubject) and row selection now navigate to./${id}/table, matching the updated UX.This all looks consistent and cohesive.
Also applies to: 187-198, 200-213, 234-237
david-roper
force-pushed
the
improve-datahub-search
branch
from
7d5edbc to
18cbb36
Compare
1 participant
Working on issue #1230
Also works on issue #1247
Summary by CodeRabbit
New Features
Improvements
Documentation
Style
✏️ Tip: You can customize this high-level summary in your review settings.