Conversation
0f8656c to
ca350df
Compare
…to add-search-filter
e75a21e to
33c68d9
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive search filtering functionality including fuzzy matching, date range filters, and aggregation-based filtering capabilities. The implementation spans both frontend (TypeScript/React) and backend (Rust) components.
- Introduces fuzzy search with configurable fuzziness levels (0-5)
- Adds date range filtering for update times with preset options (7 days, 90 days, 1 year) and custom range selection
- Implements aggregation-based filtering for categories, languages, source IDs, and types
- Updates UI components to support the new filtering features
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 26 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/search.ts | Added TypeScript interfaces for Aggregation, Aggregations, and updated MultiSourceQueryResponse to include aggregations field |
| src/stores/searchStore.ts | Extended search store with fuzzy match settings, date range filter, aggregate filter, and aggregations state management |
| src/pages/settings/shadcn-demo.tsx | Removed demo file (cleanup) |
| src/locales/zh/translation.json | Added Chinese translations for fuzzy match and filter UI elements |
| src/locales/en/translation.json | Added English translations for fuzzy match and filter UI elements |
| src/hooks/useSearch.ts | Enhanced search hook to include fuzziness, date range, and aggregate filter parameters in queries; added aggregation response handling |
| src/components/ui/slider.tsx | Extended Slider component to accept custom classNames for range and thumb elements |
| src/components/ui/multi-select.tsx | Complete rewrite using DropdownMenu instead of Popover for better UX and visual consistency |
| src/components/ui/date-picker-range.tsx | New component for date range selection using react-day-picker |
| src/components/ui/calendar.tsx | New calendar component with extensive customization options |
| src/components/Search/TimeFilter.tsx | New component providing comprehensive time-based and aggregation filtering UI |
| src/components/Search/InputControls.tsx | Integrated fuzzy match toggle with slider and TimeFilter component |
| src/components/Search/ExtensionStore.tsx | Minor style update (size-[20px] to size-5) |
| src/components/Search/AiOverview.tsx | UI consistency improvements using Button component |
| src/components/Cloud/UserProfile.tsx | UI consistency improvements using Button component |
| src-tauri/src/server/search.rs | Added query string conversion logic for filters and aggregation request body |
| src-tauri/src/search/mod.rs | Implemented aggregation merging logic and cleanup for multi-source queries |
| src-tauri/src/extension/third_party/mod.rs | Updated to include aggregations field (None for local sources) |
| src-tauri/src/extension/third_party/install/store.rs | Updated to include aggregations field (None for local sources) |
| src-tauri/src/extension/built_in/window_management/search_source.rs | Updated to include aggregations field (None for local sources) |
| src-tauri/src/extension/built_in/file_search/mod.rs | Updated to include aggregations field (None for local sources) |
| src-tauri/src/extension/built_in/calculator.rs | Updated to include aggregations field (None for local sources) |
| src-tauri/src/extension/built_in/application/without_feature.rs | Updated to include aggregations field (None for local sources) |
| src-tauri/src/extension/built_in/application/with_feature.rs | Updated to include aggregations field (None for local sources) |
| src-tauri/src/common/search.rs | Added Aggregation, AggBucket types with custom deserialization and merge_aggregations function with tests |
| pnpm-lock.yaml | Updated lucide-react dependency and added react-day-picker with date-fns dependencies |
| package.json | Updated lucide-react to 0.561.0 and added react-day-picker 9.13.0 and date-fns 4.1.0 |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
src-tauri/src/search/mod.rs:61
- Debug println! statements should be removed before merging to production. Consider using proper logging with appropriate log levels if this debugging information is needed.
if query_strings.contains_key("datasource") && !query_strings.contains_key("querysource") {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/hooks/useSearch.ts
Outdated
| setAggregations(sortedAggregations); | ||
| } else { | ||
| setAggregations(void 0); | ||
| setAggregateFilter(void 0); |
There was a problem hiding this comment.
Using "void 0" instead of "undefined" is unconventional in TypeScript/JavaScript. While technically equivalent, "undefined" is more readable and idiomatic. Consider using "undefined" directly for better code clarity.
| ); | ||
| }; | ||
|
|
||
| export default MultiSelect; |
There was a problem hiding this comment.
The component is exported both as a default export (line 115) and implicitly through the const declaration. For consistency and to avoid confusion, consider using only the default export pattern or documenting why both patterns are used.
| name: "search-store", | ||
| partialize: (state) => ({ | ||
| sourceData: state.sourceData, | ||
| fuzziness: state.fuzziness, |
There was a problem hiding this comment.
The partialize function only persists 'sourceData' and 'fuzziness', but doesn't persist 'enabledFuzzyMatch'. This could lead to inconsistent state where the fuzziness value is persisted but the enabled state is not, potentially confusing users on page reload. Consider also persisting 'enabledFuzzyMatch' for a better user experience.
| fuzziness: state.fuzziness, | |
| fuzziness: state.fuzziness, | |
| enabledFuzzyMatch: state.enabledFuzzyMatch, |
src/hooks/useSearch.ts
Outdated
| } | ||
|
|
||
| //console.log("_suggest", searchInput, response); | ||
| console.log("_suggest", searchInput, response); |
There was a problem hiding this comment.
Console.log statements should be removed before merging to production. Debug logging should use proper logging mechanisms instead of console.log.
| setFilterDateRange(void 0); | ||
|
|
||
| setAggregateFilter(void 0); |
There was a problem hiding this comment.
Using "void 0" instead of "undefined" is unconventional in TypeScript/JavaScript. While technically equivalent, "undefined" is more readable and idiomatic. Consider using "undefined" directly for better code clarity.
| setFilterDateRange(void 0); | |
| setAggregateFilter(void 0); | |
| setFilterDateRange(undefined); | |
| setAggregateFilter(undefined); |
src-tauri/src/common/search.rs
Outdated
|
|
||
| #[derive(Debug, Deserialize, Serialize, Clone)] | ||
| pub struct Aggregation { | ||
| // Frontend code needs to this to not be NULL, so we `clean_aggregations()` |
There was a problem hiding this comment.
The comment states "Frontend code needs to this to not be NULL" which appears to have a grammatical error. It should be "Frontend code needs this to not be NULL" or "Frontend code requires this to not be NULL".
| // Frontend code needs to this to not be NULL, so we `clean_aggregations()` | |
| // Frontend code needs this to not be NULL, so we `clean_aggregations()` |
src/locales/zh/translation.json
Outdated
| "filers": { | ||
| "allTime": "全部时间", | ||
| "past7Days": "过去 7 天", | ||
| "past90Days": "过去 90 天", | ||
| "past1year": "过去 1 年", | ||
| "more": "更多", | ||
| "updateTime": "更新时间", | ||
| "selectDateRange": "选择日期范围", |
There was a problem hiding this comment.
The translation key is misspelled as "filers" instead of "filters". This inconsistency appears in both English and Chinese translation files and should be corrected to "filters" throughout.
| "filers": { | |
| "allTime": "全部时间", | |
| "past7Days": "过去 7 天", | |
| "past90Days": "过去 90 天", | |
| "past1year": "过去 1 年", | |
| "more": "更多", | |
| "updateTime": "更新时间", | |
| "selectDateRange": "选择日期范围", | |
| "filters": { | |
| "allTime": "全部时间", | |
| "past7Days": "过去 7 天", | |
| "past90Days": "过去 90 天", | |
| "past1year": "过去 1 年", | |
| "more": "更多", | |
| "updateTime": "更新时间", | |
| "selectDateRange": "选择日期范围", |
src/hooks/useSearch.ts
Outdated
| console.log("queryStrings", queryStrings); | ||
|
|
There was a problem hiding this comment.
Console.log statements should be removed before merging to production. Debug logging should use proper logging mechanisms instead of console.log.
| console.log("queryStrings", queryStrings); |
| return ( | ||
| <Popover> | ||
| <PopoverTrigger asChild> | ||
| <div className="h-8 flex items-center justify-between px-2 border border-border rounded-lg"> | ||
| {selected ? ( | ||
| <div className="flex items-center gap-2"> | ||
| <span>{dayjs(selected.from).format("YYYY-MM-DD")}</span> | ||
| <span className="text-muted-foreground">-</span> | ||
| <span>{dayjs(selected.to).format("YYYY-MM-DD")}</span> |
There was a problem hiding this comment.
The component doesn't handle the case where only one of 'from' or 'to' is set. When 'selected.to' is undefined, calling dayjs(undefined).format() will return "Invalid Date". Add a check to ensure both from and to exist before rendering, or handle partial date ranges appropriately.
| return ( | |
| <Popover> | |
| <PopoverTrigger asChild> | |
| <div className="h-8 flex items-center justify-between px-2 border border-border rounded-lg"> | |
| {selected ? ( | |
| <div className="flex items-center gap-2"> | |
| <span>{dayjs(selected.from).format("YYYY-MM-DD")}</span> | |
| <span className="text-muted-foreground">-</span> | |
| <span>{dayjs(selected.to).format("YYYY-MM-DD")}</span> | |
| const hasFrom = !!selected?.from; | |
| const hasTo = !!selected?.to; | |
| const hasAnyDate = hasFrom || hasTo; | |
| return ( | |
| <Popover> | |
| <PopoverTrigger asChild> | |
| <div className="h-8 flex items-center justify-between px-2 border border-border rounded-lg"> | |
| {hasAnyDate ? ( | |
| <div className="flex items-center gap-2"> | |
| {hasFrom && ( | |
| <span>{dayjs(selected!.from).format("YYYY-MM-DD")}</span> | |
| )} | |
| {hasFrom && hasTo && ( | |
| <span className="text-muted-foreground">-</span> | |
| )} | |
| {hasTo && ( | |
| <span>{dayjs(selected!.to).format("YYYY-MM-DD")}</span> | |
| )} |
| { | ||
| key: "all-time", | ||
| label: t("search.filers.allTime"), | ||
| value: void 0, |
There was a problem hiding this comment.
Using "void 0" instead of "undefined" is unconventional in TypeScript/JavaScript. While technically equivalent, "undefined" is more readable and idiomatic. Consider using "undefined" directly for better code clarity.
| value: void 0, | |
| value: undefined, |
What does this PR do
Rationale for this change
Standards checklist