Datagraph-search-filters #295
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce enhancements to the search functionality in the application. In the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
web/src/app/(dashboard)/search/page.tsx (1)
20-28: Consider enhancing the kind parameter validationThe preprocessing logic handles string-to-array conversion, but could benefit from additional validation:
- Validate minimum array length if empty arrays are not allowed
- Add explicit handling for null/undefined cases
- Consider extracting the preprocessing logic into a reusable utility function
kind: z .preprocess((arg: unknown) => { + if (arg === null || arg === undefined) { + return []; + } if (typeof arg === "string") { return [arg]; } return arg; }, z.array(DatagraphKindSchema)) + .refine((arr) => arr.length > 0, { + message: "At least one kind must be specified", + optional: true, + }) .optional(),web/src/screens/search/SearchScreen.tsx (2)
30-30: Consider adding input validation for kind parameterWhile the implementation is correct, consider adding validation to ensure only valid
kindvalues are passed to the API. This could prevent potential runtime errors from invalid filter values.- kind: props.kind, + kind: props.kind.filter((k): k is DatagraphItemKind => + Object.values(DatagraphItemKind).includes(k)),
Line range hint
23-61: Well-structured component evolutionThe integration of the kind filter maintains the component's clean architecture while extending its functionality. The existing error handling, pagination, and search results display remain robust, making this a good example of incremental feature addition.
app/services/search/simplesearch/simple_searcher.go (1)
57-60: Consider adding observability for empty searcher casesWhile gracefully handling the empty searchers case is good, this might hide configuration issues. Consider adding logging or metrics to track these occurrences.
Add logging before returning the empty result:
if len(searchers) == 0 { + logger.Warn("no searchers available for the requested kinds", + "kinds", opts.Kinds) results := pagination.NewPageResult(p, 0, []datagraph.Item{}) return &results, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
app/services/search/simplesearch/simple_searcher.go(2 hunks)web/src/app/(dashboard)/search/page.tsx(3 hunks)web/src/screens/search/SearchScreen.tsx(3 hunks)
🔇 Additional comments (5)
web/src/app/(dashboard)/search/page.tsx (2)
47-51: Verify the datagraphSearch API contract
The function call looks correct, but let's verify the API implementation matches these parameter types.
57-57: Verify SearchScreen component prop types
The kind prop is added with a default empty array. Let's verify the SearchScreen component's type definitions.
✅ Verification successful
---
```markdown
<!-- [monologue]
[task review_file_1/1 segment_3/3]
Finalize the verification of the `kind` prop in the `SearchScreen` component based on the latest shell script outputs.
[observations]
- The `Props` interface in `SearchScreen.tsx` includes `kind: DatagraphItemKind[]`, indicating that the `kind` prop is required and expects an array of `DatagraphItemKind`.
- The component usage `kind={params.kind ?? []}` correctly provides a default empty array if `params.kind` is undefined.
- The shell script successfully located the `Props` interface and confirmed the usage of the `kind` prop within the `SearchScreen` component.
- No syntax or type definition issues were found related to the `kind` prop.
[analysis]
- The implementation aligns with the review comment's intention to add the `kind` prop with a default value.
- The prop type is correctly defined and utilized within the component.
- No further verification is necessary as the current evidence supports the correctness of the change.
[actions]
- Conclude the verification process by marking it as verified.
-->SearchScreen component correctly handles the kind prop
The SearchScreen component accepts the kind prop as defined. No issues found.
</details>
<details>
<summary>🏁 Scripts executed</summary>
The following scripts were executed for the analysis:
Script:
```shell
#!/bin/bash
# Search for SearchScreen component definition and its Props interface
ast-grep --pattern 'interface Props {
$$$
}' -l typescript
rg -t typescript "^export.*interface.*Props" -g "*/SearchScreen.tsx"
Length of output: 262
Script:
#!/bin/bash
# Search for SearchScreen component definition and its Props interface
ast-grep --pattern 'interface Props {
$$$
}' -l tsx
rg -t tsx '^export\s+interface\s+Props' --glob '*SearchScreen.tsx'
Length of output: 382
Script:
#!/bin/bash
# Extract the props used by the SearchScreen component
# Attempt to find function-based component definitions
ast-grep --pattern 'function SearchScreen($_) { $$$ }' -l tsx
# Attempt to find arrow function-based component definitions
ast-grep --pattern 'const SearchScreen = ($_): { $$$ } => { $$$ }' -l tsx
# Fallback: Display the entire SearchScreen.tsx file for manual inspection
cat $(fd --type f --extension tsx 'SearchScreen.tsx')
Length of output: 3993
web/src/screens/search/SearchScreen.tsx (2)
7-10: LGTM: Clean import restructuring
The import restructuring improves code organization and properly imports the new DatagraphItemKind type needed for the search filtering feature.
19-19: Verify API schema alignment
The kind prop type looks correct, but let's verify it aligns with the API schema definition.
✅ Verification successful
DatagraphItemKind type correctly aligned with API schema
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the DatagraphItemKind type definition matches the API schema
# Search for the DatagraphItemKind type definition
ast-grep --pattern 'type DatagraphItemKind = $_'
# Search for API schema usage of this type
rg -A 5 "DatagraphItemKind" --type ts
Length of output: 10613
app/services/search/simplesearch/simple_searcher.go (1)
32-34: LGTM! Verify post searcher compatibility with threads
The mapping looks correct, with both posts and threads using the same searcher implementation.
Let's verify that the postSearcher implementation properly handles both kinds:
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (24)
web/src/recipes/tooltip.ts (1)
4-27: Consider enhancing accessibility and maintainability.The tooltip implementation is solid with good use of design tokens and reasonable animation timings. However, there are a few improvements that could be made:
Consider applying these enhancements:
export const tooltip = defineSlotRecipe({ className: "tooltip", slots: tooltipAnatomy.keys(), base: { content: { background: "bg.default", borderRadius: "l2", boxShadow: "sm", color: "fg.default", fontWeight: "semibold", px: "3", py: "2", textStyle: "xs", + lineHeight: "normal", maxWidth: "2xs", zIndex: "tooltip", + role: "tooltip", + ariaLabel: "Tooltip", _open: { - animation: "fadeIn 0.25s ease-out", + animation: "fadeIn token(durations.normal) ease-out", }, _closed: { - animation: "fadeOut 0.2s ease-out", + animation: "fadeOut token(durations.fast) ease-out", }, }, }, });The changes above:
- Add line height constraint for better text readability
- Include ARIA attributes for improved accessibility
- Use design tokens for animation durations to maintain consistency
web/src/components/ui/toggle-group.tsx (3)
15-22: Consider improving type readability with type aliasesWhile the type composition is correct, consider extracting the nested Assign types into a separate type alias for better readability and maintainability.
+type RootProviderBaseProps = Assign<HTMLStyledProps<"div">, ToggleGroup.RootProviderBaseProps>; +type RootProviderProps = Assign<RootProviderBaseProps, ToggleGroupVariantProps>; + export const RootProvider = withProvider< HTMLDivElement, - Assign< - Assign<HTMLStyledProps<"div">, ToggleGroup.RootProviderBaseProps>, - ToggleGroupVariantProps - > + RootProviderProps >(ToggleGroup.RootProvider, "root");
24-31: Apply consistent type handling with RootProviderFor consistency with the RootProvider component, consider applying the same type organization pattern.
+type RootBaseProps = Assign<HTMLStyledProps<"div">, ToggleGroup.RootBaseProps>; +type RootComponentProps = Assign<RootBaseProps, ToggleGroupVariantProps>; + export const Root = withProvider< HTMLDivElement, - Assign< - Assign<HTMLStyledProps<"div">, ToggleGroup.RootBaseProps>, - ToggleGroupVariantProps - > + RootComponentProps >(ToggleGroup.Root, "root");
1-38: Consider adding component documentationSince this toggle group component is being used for search filters, it would be helpful to add JSDoc comments describing:
- The component's purpose in the search filter context
- Usage examples with the new kind parameter
- Available variant props and their effects
This will help other developers understand how to use this component correctly in the search filter implementation.
web/src/app/(dashboard)/search/page.tsx (2)
21-29: Add validation for kind parameterWhile the preprocessing handles string to array conversion, consider adding validation to:
- Filter out empty strings
- Ensure unique values
- Validate minimum/maximum array length if applicable
kind: z .preprocess((arg: unknown) => { if (typeof arg === "string") { + if (!arg.trim()) return []; return [arg]; } return arg; - }, z.array(DatagraphKindSchema)) + }, z.array(DatagraphKindSchema) + .transform(kinds => [...new Set(kinds.filter(k => k.trim()))]) + .refine(kinds => kinds.length <= 10, "Maximum 10 kinds allowed")) .optional(),
40-49: Remove obsolete TODO commentThe TODO comment on line 40 can be removed as the check for empty query is already implemented correctly below it.
- // TODO: Don't run this query if query is empty const { data } = params.qweb/src/components/ui/tooltip.tsx (3)
12-15: Consider adding JSDoc documentationWhile the implementation is correct, adding documentation about the provider's purpose and usage would improve maintainability.
+/** + * Provider component for the Tooltip context. + * Manages the shared state and configuration for all tooltip components. + */ export type RootProviderProps = ComponentProps<typeof RootProvider>;
17-20: Consider adding JSDoc documentationThe implementation is solid, but would benefit from documentation explaining its relationship with RootProvider.
+/** + * Root component for the Tooltip. + * Must be wrapped by RootProvider to function correctly. + */ export type RootProps = ComponentProps<typeof Root>;
22-45: Consider grouping related components and adding documentationWhile the implementation is correct, consider organizing these components more clearly and adding documentation for better maintainability.
Consider grouping related components and adding documentation:
+/** + * Visual Components + */ export const Arrow = withContext<...>; export const ArrowTip = withContext<...>; +/** + * Structural Components + */ export const Content = withContext<...>; export const Positioner = withContext<...>; export const Trigger = withContext<...>;web/src/components/ui/form/DatagraphKindFilterField.tsx (5)
9-18: Add JSDoc comments to improve type documentationConsider adding JSDoc comments to the
CollectionItemtype to document the purpose of each property, especially for consumers of this component.+/** + * Represents an item in the filter toggle group + */ type CollectionItem = { + /** Display text for the filter item */ label: string; + /** React element to display as an icon */ icon: JSX.Element; + /** Tooltip text explaining the filter option */ description: string; + /** Unique identifier for the filter option */ value: string; };
46-48: Consider adding a small delay to prevent tooltip flickerThe
openDelay={0}might cause tooltip flicker on accidental hovers. Consider adding a small delay (e.g., 200ms) for better user experience.<Tooltip.Root lazyMount - openDelay={0} + openDelay={200}
40-44: Enhance accessibility with aria-describedbyWhile
aria-labelis provided, connecting the toggle item with its tooltip usingaria-describedbywould improve accessibility.<ToggleGroup.Item key={item.value} value={item.value} aria-label={item.description} + aria-describedby={`tooltip-${item.value}`} >Then update the tooltip content:
-<Tooltip.Content>{item.description}</Tooltip.Content> +<Tooltip.Content id={`tooltip-${item.value}`}>{item.description}</Tooltip.Content>
48-52: Document the magic number in tooltip positioningThe hardcoded shift value (-48) should be documented or moved to a named constant for better maintainability.
+// The shift value accounts for the toggle group item's width to align tooltip +const TOOLTIP_SHIFT = -48; + positioning={{ slide: true, - shift: -48, + shift: TOOLTIP_SHIFT, placement: "right-end", }}
20-77: Consider extracting tooltip configuration to a shared constantThe tooltip configuration (positioning, delay, etc.) might be reused across other components. Consider extracting it to a shared configuration object for consistency.
// In a shared config file export const FILTER_TOOLTIP_CONFIG = { lazyMount: true, openDelay: 200, positioning: { slide: true, shift: -48, placement: "right-end" as const, }, }; // Then in the component: <Tooltip.Root {...FILTER_TOOLTIP_CONFIG}>web/src/screens/search/SearchScreen.tsx (4)
21-24: Consider adding type annotations for destructured valuesWhile the code works correctly, adding explicit type annotations would improve type safety and code documentation.
- const { form, error, isLoading, data, handlers } = useSearchScreen(props); - const { query, page, results } = data; + const { form, error, isLoading, data, handlers } = useSearchScreen(props); + const { query, page, results }: { query: string; page: number; results: DatagraphSearchOKResponse | null } = data;
44-48: Track TODO for type fixThe TODO comment indicates a type issue with the boxShadow property that needs addressing.
Would you like me to create a GitHub issue to track this type-related TODO?
65-78: Simplify disabled state conditionThe disabled state condition can be simplified for better readability.
- disabled={query ? false : true} + disabled={!query}🧰 Tools
🪛 Biome
[error] 74-74: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
102-117: Remove redundant optional chainingThe optional chaining on
results?.itemsis redundant since we already check forresultsexistence in the condition.- {results && results?.items.length > 0 ? ( + {results && results.items.length > 0 ? (web/src/screens/search/useSearch.ts (3)
80-82: Reset the page number when a new search is performedWhen a new search query is submitted, consider resetting the
pageto1so that users start at the first page of results. This ensures users see the first set of results for their new search.Apply this diff to implement the change:
const handleSearch = form.handleSubmit((data) => { setQuery(data.q); + setPage(1); });
84-87: Reset the page number when resetting the searchWhen resetting the search form, also reset the
pagestate to1to ensure pagination is reset.Apply this diff:
const handleReset = async () => { form.reset(); setQuery(null); + setPage(1); };
75-79: Simplify loading state managementInstead of using a
useEffectto synchronizeisLoadingtoisSearchLoading, you can directly useisLoading ?? falsewithout additional state.Apply this diff to simplify the code:
-const [isSearchLoading, setLoading] = useState(false); -useEffect(() => { - setLoading(isLoading ?? false); -}, [isLoading]); +const isSearchLoading = isLoading ?? false;web/src/recipes/toggle-group.ts (3)
96-148: Refactor repeated styles in size variants for maintainability.The
itemstyles within each size variant (xs,sm,md,lg) have repetitive code. Consider abstracting common styles into a base style or using a function to generate size-specific adjustments. This will improve maintainability and reduce the likelihood of inconsistencies.
82-93: Ensure consistent gap usage inghostvariant.In the
ghostvariant, therootslot has agap: "1"property, whereas theoutlinevariant does not. Verify whether this discrepancy is intentional and aligns with the design specifications. Consistency between variants enhances the user experience.
7-54: Review base styles for comprehensive state coverage.The base styles for
rootanditemslots define various states like_hoverand_disabled. Ensure that all necessary interactive states (e.g.,_focus,_active,_pressed) are included as per the design requirements to enhance accessibility and user interaction feedback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
web/panda.config.ts(2 hunks)web/src/app/(dashboard)/search/page.tsx(2 hunks)web/src/components/ui/form/DatagraphKindFilterField.tsx(1 hunks)web/src/components/ui/toggle-group.tsx(1 hunks)web/src/components/ui/tooltip.tsx(1 hunks)web/src/recipes/toggle-group.ts(1 hunks)web/src/recipes/tooltip.ts(1 hunks)web/src/screens/search/SearchScreen.tsx(1 hunks)web/src/screens/search/useSearch.ts(1 hunks)web/styled-system/recipes/toggle-group.d.ts(2 hunks)web/styled-system/recipes/toggle-group.mjs(2 hunks)web/styled-system/recipes/tooltip.d.ts(1 hunks)web/styled-system/recipes/tooltip.mjs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- web/styled-system/recipes/tooltip.d.ts
🧰 Additional context used
🪛 Biome
web/src/screens/search/SearchScreen.tsx
[error] 74-74: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🔇 Additional comments (20)
web/src/recipes/tooltip.ts (1)
1-2: LGTM! Clean and focused imports.
The imports are correctly scoped to the required functionality from Ark UI and Panda CSS.
web/styled-system/recipes/toggle-group.d.ts (1)
13-13: LGTM: Size variant enhancement
The addition of the "xs" size option provides more flexibility for compact layouts.
web/src/components/ui/toggle-group.tsx (3)
3-10: LGTM: Imports are well-structured and properly typed
The imports are correctly organized, bringing in necessary types and utilities from both ark-ui and styled-system.
33-36: LGTM: Item component implementation is clean and well-typed
The Item component correctly uses withContext and has a clear type composition.
38-38: LGTM: Clean context export with good naming
The context export uses a clear alias that follows good naming conventions.
web/src/app/(dashboard)/search/page.tsx (2)
53-55: LGTM! Consistent prop handling
The addition of initialKind prop follows the established pattern of other initial props, with appropriate default values.
13-14: Verify if force-dynamic rendering is necessary
The force-dynamic setting disables page caching and forces server-side rendering on every request. While this ensures real-time search results, it could impact performance and increase server load.
✅ Verification successful
force-dynamic rendering is appropriately applied to ensure real-time data without caching.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any cache-related comments or configurations
rg -A 3 "cache|revalidate" src/app
# Check for other dynamic exports to understand the caching strategy
ast-grep --pattern 'export const dynamic = $_'
Length of output: 570
web/styled-system/recipes/toggle-group.mjs (1)
59-59: LGTM: Size variant addition
The addition of the "xs" size variant is a good enhancement that provides more flexibility for smaller toggle group components. The sizing scale (xs, sm, md, lg) follows the conventional order.
web/src/components/ui/tooltip.tsx (3)
1-7: LGTM! Clean and well-organized imports
The imports are properly structured with clear separation between types and values, following React best practices with the "use client" directive.
9-10: LGTM! Good use of style context
The createStyleContext utility provides a clean way to maintain consistent styling across tooltip components.
47-47: LGTM! Clean context export
The context export uses a clear alias that maintains consistency with the component's public API.
web/src/components/ui/form/DatagraphKindFilterField.tsx (1)
1-8: LGTM! Well-organized imports
The imports are properly organized with external dependencies followed by internal ones, and all imports are utilized in the implementation.
web/src/screens/search/SearchScreen.tsx (4)
6-19: LGTM! Well-organized imports
The imports are logically grouped and follow a clear structure.
52-64: LGTM! Good UX for search reset
The conditional rendering of the reset button and its styling provide clear visual feedback to users.
27-33: Verify form action compatibility with client-side handling
The form has both an onSubmit handler and an action attribute. This might cause conflicts between client-side and traditional form submission.
81-100: Verify DatagraphItemKind enum values
Ensure that the DatagraphItemKind values match the API schema definition.
web/panda.config.ts (1)
24-25: LGTM! Verify imported recipe files.
The additions follow the existing patterns and maintain consistent organization. The imports and slot recipes are properly integrated into the configuration.
Let's verify the existence and basic structure of the imported recipe files:
Also applies to: 300-301
✅ Verification successful
Verified: Imported recipe files exist and export the expected constants.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and structure of the new recipe files
# Check if the recipe files exist and contain the expected exports
echo "Checking toggle-group.ts..."
fd --type f "toggle-group.ts" "web/src/recipes" --exec cat {} \; | grep -E "export const toggleGroup"
echo "Checking tooltip.ts..."
fd --type f "tooltip.ts" "web/src/recipes" --exec cat {} \; | grep -E "export const tooltip"
Length of output: 404
web/src/recipes/toggle-group.ts (3)
33-39: Verify the use of the _on state selector.
The _on state selector is used to style the toggle item's active state. Please confirm that _on is the correct state identifier in your styling system. If not, consider using a standard state like _active or a proper attribute selector to ensure expected behavior.
66-71: Confirm the usage of divideX and divideY properties.
The properties divideX and divideY are utilized to apply dividers between toggle items. Ensure that these properties are supported by your styling system (@pandacss/dev), as they are not standard CSS properties. If unsupported, you may need to implement an alternative approach for the dividers.
1-2: Imports align with module dependencies.
The import statements correctly bring in toggleGroupAnatomy from @ark-ui/anatomy and defineSlotRecipe from @pandacss/dev, which are necessary for defining the toggle group recipe.
| @@ -23,7 +23,7 @@ export type ToggleGroupVariantProps = { | |||
|
|
|||
| export interface ToggleGroupRecipe { | |||
| __type: ToggleGroupVariantProps | |||
| (props?: ToggleGroupVariantProps): Pretty<Record<"root" | "item" | "root" | "item", string>> | |||
| (props?: ToggleGroupVariantProps): Pretty<Record<"root" | "item" | "root" | "item" | "root" | "item", string>> | |||
There was a problem hiding this comment.
Remove duplicate entries in return type
The return type contains redundant entries where "root" | "item" is repeated three times. TypeScript automatically removes duplicates from union types, so this repetition is unnecessary and may confuse developers.
Apply this fix:
-(props?: ToggleGroupVariantProps): Pretty<Record<"root" | "item" | "root" | "item" | "root" | "item", string>>
+(props?: ToggleGroupVariantProps): Pretty<Record<"root" | "item", string>>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (props?: ToggleGroupVariantProps): Pretty<Record<"root" | "item" | "root" | "item" | "root" | "item", string>> | |
| (props?: ToggleGroupVariantProps): Pretty<Record<"root" | "item", string>> |
| [ | ||
| "item", | ||
| "toggleGroup__item" | ||
| ], | ||
| [ | ||
| "root", | ||
| "toggleGroup__root" | ||
| ], |
There was a problem hiding this comment.
Remove duplicate slot entries
The toggleGroupSlotNames array contains duplicate entries for both "root" and "item" slots. Duplicate slot definitions are redundant and could potentially cause issues with style application or slot resolution.
Apply this diff to remove the duplicates:
const toggleGroupSlotNames = [
[
"root",
"toggleGroup__root"
],
[
"item",
"toggleGroup__item"
],
- [
- "root",
- "toggleGroup__root"
- ],
- [
- "item",
- "toggleGroup__item"
- ],
- [
- "root",
- "toggleGroup__root"
- ],
[
"item",
"toggleGroup__item"
]
]Committable suggestion skipped: line range outside the PR's diff.
| [ | ||
| "content", | ||
| "tooltip__content" | ||
| ], | ||
| [ | ||
| "trigger", | ||
| "tooltip__trigger" | ||
| ], | ||
| [ | ||
| "arrow", | ||
| "tooltip__arrow" | ||
| ], | ||
| [ | ||
| "arrowTip", | ||
| "tooltip__arrowTip" | ||
| ], | ||
| [ | ||
| "positioner", | ||
| "tooltip__positioner" | ||
| ], |
There was a problem hiding this comment.
Remove duplicate tooltip slot entries
The code contains redundant entries in the tooltipSlotNames array where each slot ("trigger", "arrow", "arrowTip", "positioner", "content") appears three times. This duplication:
- Increases memory usage unnecessarily
- Creates redundant slot functions through
tooltipSlotFns.map() - Makes maintenance more difficult
Apply this diff to remove the duplicates:
const tooltipSlotNames = [
[
"trigger",
"tooltip__trigger"
],
[
"arrow",
"tooltip__arrow"
],
[
"arrowTip",
"tooltip__arrowTip"
],
[
"positioner",
"tooltip__positioner"
],
[
"content",
"tooltip__content"
],
- [
- "trigger",
- "tooltip__trigger"
- ],
- [
- "arrow",
- "tooltip__arrow"
- ],
- [
- "arrowTip",
- "tooltip__arrowTip"
- ],
- [
- "positioner",
- "tooltip__positioner"
- ],
- [
- "content",
- "tooltip__content"
- ],
- [
- "trigger",
- "tooltip__trigger"
- ],
- [
- "arrow",
- "tooltip__arrow"
- ],
- [
- "arrowTip",
- "tooltip__arrowTip"
- ],
- [
- "positioner",
- "tooltip__positioner"
- ],
- [
- "content",
- "tooltip__content"
- ]
]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [ | |
| "content", | |
| "tooltip__content" | |
| ], | |
| [ | |
| "trigger", | |
| "tooltip__trigger" | |
| ], | |
| [ | |
| "arrow", | |
| "tooltip__arrow" | |
| ], | |
| [ | |
| "arrowTip", | |
| "tooltip__arrowTip" | |
| ], | |
| [ | |
| "positioner", | |
| "tooltip__positioner" | |
| ], | |
| [ | |
| "content", | |
| "tooltip__content" | |
| ], | |
| [ | |
| "trigger", | |
| "tooltip__trigger" | |
| ], | |
| [ | |
| "arrow", | |
| "tooltip__arrow" | |
| ], | |
| [ | |
| "arrowTip", | |
| "tooltip__arrowTip" | |
| ], | |
| [ | |
| "positioner", | |
| "tooltip__positioner" | |
| ] |
| render={({ formState, field }) => { | ||
| function handleChangeFilter({ value }: ToggleGroupValueChangeDetails) { | ||
| field.onChange(value); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for form state
The component should handle and display form errors to provide feedback to users.
render={({ formState, field }) => {
+ const error = formState.errors[props.name];
function handleChangeFilter({ value }: ToggleGroupValueChangeDetails) {
field.onChange(value);
}
+
+ return (
+ <div>
+ {/* Existing ToggleGroup implementation */}
+ {error && (
+ <div role="alert" style={{ color: 'red', marginTop: '4px' }}>
+ {error.message}
+ </div>
+ )}
+ </div>
+ );Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
web/src/components/site/Navigation/Anchors/Search.tsx (1)
12-22: Consider adding prop validationWhile the component is well-structured, consider being more explicit about which props from LinkButtonStyleProps are supported to prevent passing unwanted props through spreading.
-type Props = AnchorProps & LinkButtonStyleProps; +type Props = AnchorProps & Pick<LinkButtonStyleProps, 'size' | 'variant'>;web/src/components/search/Search/Search.tsx (1)
17-17: Consider cleanup of unused handlerIf the client-side form submission is no longer needed, consider removing the
handleSearchhandler from theuseSearchhook instead of commenting out its usage.web/src/api/openapi-server/datagraph.ts (1)
Line range hint
24-44: Inconsistency Between Type Definition and ImplementationThe function signature requires
paramsto be provided (params: DatagraphSearchParams), but the implementation still handles undefined values withparams || {}. This creates a disconnect between the type contract and runtime behavior.Consider either:
- Maintaining the optional parameter in the type definition to match the implementation, or
- Removing the null-safe handling since the type system ensures params will be provided
-export const getDatagraphSearchUrl = (params: DatagraphSearchParams) => { +export const getDatagraphSearchUrl = (params?: DatagraphSearchParams) => { const normalizedParams = new URLSearchParams(); - Object.entries(params || {}).forEach(([key, value]) => { + Object.entries(params ?? {}).forEach(([key, value]) => {web/src/api/openapi-client/datagraph.ts (1)
33-34: Simplify key generation logicSince
paramsis now required, the nullish check is redundant and can be simplified.-export const getDatagraphSearchKey = (params: DatagraphSearchParams) => - [`/datagraph`, ...(params ? [params] : [])] as const; +export const getDatagraphSearchKey = (params: DatagraphSearchParams) => + [`/datagraph`, params] as const;web/src/screens/search/useSearch.ts (1)
24-28: Consider enhancing the form schema validation.The current schema provides basic validation, but consider adding:
- Maximum length constraint for the search term to prevent excessive queries
- Trim whitespace from the search term
- Validation for maximum number of kinds that can be selected
export const FormSchema = z.object({ - q: z.string().min(1, { message: "Please enter a search term" }), + q: z.string() + .min(1, { message: "Please enter a search term" }) + .max(100, { message: "Search term is too long" }) + .transform(s => s.trim()), - kind: z.array(DatagraphKindSchema).optional(), + kind: z.array(DatagraphKindSchema) + .max(5, { message: "Maximum 5 kinds can be selected" }) + .optional(), });app/transports/http/bindings/datagraph.go (1)
Line range hint
29-29: Consider making page size configurable.The
datagraphSearchPageSizeconstant could be made configurable through dependency injection or configuration to allow for flexibility in different environments or use cases.Consider modifying the
Datagraphstruct and constructor:type Datagraph struct { searcher searcher.Searcher + pageSize int } func NewDatagraph( searcher searcher.Searcher, + pageSize int, ) Datagraph { return Datagraph{ searcher: searcher, + pageSize: pageSize, } }web/src/api/openapi-schema/index.ts (1)
358-358: LGTM! The new export is correctly placed.The addition of
requiredSearchQueryParameterexport is properly ordered alphabetically and aligns with the search functionality enhancements.Note: This is an auto-generated file from the OpenAPI specification. Any changes to the schema should be made in the source OpenAPI definition file instead of directly modifying this file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
api/openapi.yamlis excluded by!**/*.yaml
📒 Files selected for processing (13)
app/transports/http/bindings/datagraph.go(1 hunks)app/transports/http/openapi/server_gen.go(5 hunks)web/src/api/openapi-client/datagraph.ts(2 hunks)web/src/api/openapi-schema/datagraphSearchParams.ts(1 hunks)web/src/api/openapi-schema/index.ts(1 hunks)web/src/api/openapi-schema/requiredSearchQueryParameter.ts(1 hunks)web/src/api/openapi-server/datagraph.ts(2 hunks)web/src/app/(dashboard)/search/page.tsx(2 hunks)web/src/components/search/Search/Search.tsx(1 hunks)web/src/components/site/Navigation/Anchors/Search.tsx(1 hunks)web/src/components/site/Navigation/DesktopCommandBar.tsx(2 hunks)web/src/screens/search/SearchScreen.tsx(1 hunks)web/src/screens/search/useSearch.ts(1 hunks)
🔥 Files not summarized due to errors (1)
- app/transports/http/openapi/server_gen.go: Error: Server error: no LLM provider could handle the message
✅ Files skipped from review due to trivial changes (1)
- web/src/api/openapi-schema/requiredSearchQueryParameter.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/app/(dashboard)/search/page.tsx
- web/src/screens/search/SearchScreen.tsx
🔇 Additional comments (22)
web/src/components/site/Navigation/Anchors/Search.tsx (4)
1-8: LGTM! Well-structured imports and constants
The imports are clean and specific, and the constants provide good centralization of navigation-related values.
10-10: LGTM! Clean type composition
Good use of type intersection to combine AnchorProps and LinkButtonStyleProps.
24-33: LGTM! Well-structured menu item component
The component is focused, reuses constants effectively, and has controlled props without unnecessary spreading.
1-33: Verify integration with new search filter functionality
Since this PR introduces datagraph search filters, we should verify that this navigation component properly leads to the updated search screen with filter capabilities.
web/src/api/openapi-schema/datagraphSearchParams.ts (2)
12-12: LGTM!
The import statement follows the consistent pattern with other imports in the file.
18-18: Verify the impact of making the search query required
The change from optional to required search query parameter represents a breaking change that could affect existing code. Please ensure all consumers of DatagraphSearchParams are updated to handle this requirement.
✅ Verification successful
Verified: All usages of DatagraphSearchParams correctly include the required q parameter.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of DatagraphSearchParams to verify they handle required query parameter
# Search for imports and usages of DatagraphSearchParams
echo "Searching for DatagraphSearchParams usage patterns..."
rg -A 3 "DatagraphSearchParams" --type ts --type tsx
# Search for potential partial type usage that might break
echo "Searching for potential Partial<DatagraphSearchParams> usage..."
rg "Partial<.*DatagraphSearchParams.*>" --type ts --type tsx
# Search for object literals that might be affected
echo "Searching for object literals passed to search functions..."
ast-grep --pattern 'datagraphSearch({ $$$: $_$ })'
Length of output: 607
Script:
#!/bin/bash
# Description: Corrected script to find all usages of DatagraphSearchParams to verify they handle required query parameter
# Search for imports and usages of DatagraphSearchParams
echo "Searching for DatagraphSearchParams usage patterns..."
rg -A 3 "DatagraphSearchParams" --type ts --type tsx
# Search for potential partial type usage that might break
echo "Searching for potential Partial<DatagraphSearchParams> usage..."
rg "Partial<.*DatagraphSearchParams.*>" --type ts --type tsx
# Search for object literals that might be affected
echo "Searching for object literals passed to search functions..."
ast-grep --pattern 'datagraphSearch({ $$$: $_$ })'
Length of output: 607
Script:
#!/bin/bash
# Description: Corrected script to find all usages of DatagraphSearchParams to verify they handle required query parameter
# Search for imports and usages of DatagraphSearchParams
echo "Searching for DatagraphSearchParams usage patterns..."
rg "DatagraphSearchParams" --glob "*.ts" --glob "*.tsx" -A 3
# Search for potential partial type usage that might break
echo "Searching for potential Partial<DatagraphSearchParams> usage..."
rg "Partial<.*DatagraphSearchParams.*>" --glob "*.ts" --glob "*.tsx"
# Search for object literals that might be affected
echo "Searching for object literals passed to search functions..."
ast-grep --pattern 'datagraphSearch({ $$$: $_$ })'
Length of output: 2983
Script:
#!/bin/bash
# Description: Verify that all instances of DatagraphSearchParams include the required 'q' parameter
# Search for usages of DatagraphSearchParams without the 'q' parameter in object literals
echo "Searching for DatagraphSearchParams usages missing the 'q' parameter..."
ast-grep --pattern '
datagraphSearch({
$$$
})
where not any($$.key == "q")
'
# Alternatively, search for function calls where DatagraphSearchParams is passed without 'q'
echo "Searching for function calls with DatagraphSearchParams missing 'q'..."
ast-grep --pattern '
someFunctionCall({
params: {
$$$$
}
})
where not any($$.key == "q")
'
Length of output: 517
web/src/components/site/Navigation/DesktopCommandBar.tsx (2)
9-9: LGTM! Clean import addition
The SearchAnchor import follows the established import organization pattern.
31-31: Verify the search navigation behavior
The switch from Search to SearchAnchor component suggests a change in the navigation pattern. Let's verify that all search-related routes and navigation flows are properly maintained.
web/src/components/search/Search/Search.tsx (2)
Line range hint 59-64: Verify loading state behavior with server-side handling
The submit button shows a loading state based on data.isLoading, but this might not work as expected with server-side form handling. Consider how the loading state should behave during form submission.
#!/bin/bash
# Check the implementation of loading state in useSearch hook
rg "isLoading" --type ts --type tsx -C 517-17: Verify the intentional shift to server-side form handling
The form submission handler has been commented out while keeping action="/search". This suggests a shift from client-side to server-side form handling. Please verify if this is intentional and aligns with the search functionality changes across the application.
web/src/api/openapi-server/datagraph.ts (2)
Line range hint 48-57: Verify Parameter Usage in Consuming Code
The parameter is now required. Ensure all calling code provides the required DatagraphSearchParams.
Line range hint 1-13:
This file is auto-generated by orval. Manual changes may be overwritten during the next generation. Consider updating the OpenAPI specification instead.
✅ Verification successful
This file is auto-generated by orval and should not be modified directly. Any manual changes will be lost when the code is regenerated from the OpenAPI specification. Instead, update the OpenAPI spec and regenerate the code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this is indeed a generated file that shouldn't be modified
rg -l "Generated by orval" | while read -r file; do
echo "Found generated file: $file"
# Show generation configuration if it exists
fd -g 'orval.config.*' -x cat {}
done
Length of output: 631
Script:
#!/bin/bash
# Corrected script to check for generated files and display generation configurations
rg -l "Generated by orval" | while read -r file; do
echo "Found generated file: $file"
# Show generation configuration if it exists
config_file=$(fd -g 'orval.config.*' . --exclude-dir=node_modules --exclude-dir=dist)
if [ -f "$config_file" ]; then
echo "Displaying contents of $config_file:"
cat "$config_file"
else
echo "No generation configuration found."
fi
done
Length of output: 255001
web/src/api/openapi-client/datagraph.ts (2)
Line range hint 50-71: LGTM! Type-safe implementation
The hook implementation correctly handles the required params while maintaining proper error typing and SWR configuration options.
25-31: Verify all callers handle required params
The change from optional to required params is a breaking change that improves type safety. However, we should verify that all callers are updated to handle this requirement.
✅ Verification successful
All datagraphSearch callers handle the required params correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of datagraphSearch to verify they provide params
# Expected: All calls should include params argument
# Search for direct function calls
rg "datagraphSearch\(" --type ts
# Search for potential destructured usage
rg "import.*\{.*datagraphSearch.*\}.*from.*datagraph" --type ts
Length of output: 365
web/src/screens/search/useSearch.ts (1)
90-103: LGTM! Well-structured return object.
The return object is cleanly organized with clear separation of concerns between form state, loading state, data, and handlers.
app/transports/http/bindings/datagraph.go (2)
Line range hint 1-150: Implementation demonstrates robust error handling and type safety.
The code shows good practices:
- Proper error wrapping with context preservation
- Safe type conversions with error checking
- Exhaustive type switching for datagraph items
48-48: Verify nil query handling in the searcher implementation.
The removal of the nil check for request.Params.Q could lead to potential issues if the underlying searcher doesn't properly handle nil or empty queries.
Let's verify the searcher's implementation:
app/transports/http/openapi/server_gen.go (5)
3003-3005: LGTM: New RequiredSearchQuery type is well-defined
The new type definition is properly documented and follows the codebase conventions.
3461-3461: LGTM: DatagraphSearchParams structure is properly updated
The structure is well-defined with:
- Required
Qfield using the new RequiredSearchQuery type - Optional
Kindfield with appropriate tags and pointer type
Also applies to: 3463-3464
8194-8201: LGTM: Query parameter handling is robust
The implementation properly:
- Handles parameter styling and location
- Parses query parameters
- Propagates errors appropriately
19601-19605: LGTM: Required parameter binding is well-implemented
The implementation correctly:
- Binds the required "q" parameter
- Returns descriptive HTTP 400 error on invalid format
- Uses proper error handling
28739-29087: Skipping review of generated swagger specification
This section contains base64 encoded swagger specification which is machine generated.
| const handleSearch = form.handleSubmit((data) => { | ||
| setQuery(data.q); | ||
| }); | ||
|
|
||
| if (!data) { | ||
| return { | ||
| ready: false as const, | ||
| error, | ||
| }; | ||
| } | ||
| const handleReset = async () => { | ||
| form.reset(); | ||
| setQuery(null); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and async function usage.
The event handlers could be enhanced:
handleResetis marked async but doesn't use await- Neither function has error handling
- const handleSearch = form.handleSubmit((data) => {
- setQuery(data.q);
- });
+ const handleSearch = form.handleSubmit(
+ (data) => {
+ try {
+ setQuery(data.q);
+ } catch (error) {
+ console.error('Failed to update search query:', error);
+ }
+ }
+ );
- const handleReset = async () => {
+ const handleReset = () => {
+ try {
form.reset();
setQuery(null);
+ } catch (error) {
+ console.error('Failed to reset form:', error);
+ }
};Committable suggestion skipped: line range outside the PR's diff.
| // NOTE: Because useQueryState does not support proper URL query arrays, we | ||
| // modify the "kind" query parameter array directly using browser APIs. | ||
| useEffect(() => { | ||
| if (kind) { | ||
| const url = new URL(window.location.href); | ||
|
|
||
| url.searchParams.delete("kind"); | ||
| kind.forEach((k) => url.searchParams.append("kind", k)); | ||
|
|
||
| window.history.replaceState({}, "", url.toString()); | ||
| } | ||
| }, [kind]); |
There was a problem hiding this comment.
URL manipulation could lead to issues.
The current implementation has several potential problems:
- Direct manipulation of
window.historybypasses the router's history management - No cleanup on component unmount
- Possible race conditions with rapid updates
Consider using the router's API instead:
- useEffect(() => {
- if (kind) {
- const url = new URL(window.location.href);
-
- url.searchParams.delete("kind");
- kind.forEach((k) => url.searchParams.append("kind", k));
-
- window.history.replaceState({}, "", url.toString());
- }
- }, [kind]);
+ const router = useRouter();
+ useEffect(() => {
+ const params = new URLSearchParams(window.location.search);
+ params.delete("kind");
+ kind?.forEach((k) => params.append("kind", k));
+
+ const timeout = setTimeout(() => {
+ router.replace(`${window.location.pathname}?${params.toString()}`, {
+ scroll: false,
+ });
+ }, 100);
+
+ return () => clearTimeout(timeout);
+ }, [kind, router]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // NOTE: Because useQueryState does not support proper URL query arrays, we | |
| // modify the "kind" query parameter array directly using browser APIs. | |
| useEffect(() => { | |
| if (kind) { | |
| const url = new URL(window.location.href); | |
| url.searchParams.delete("kind"); | |
| kind.forEach((k) => url.searchParams.append("kind", k)); | |
| window.history.replaceState({}, "", url.toString()); | |
| } | |
| }, [kind]); | |
| // NOTE: Because useQueryState does not support proper URL query arrays, we | |
| // modify the "kind" query parameter array directly using browser APIs. | |
| const router = useRouter(); | |
| useEffect(() => { | |
| const params = new URLSearchParams(window.location.search); | |
| params.delete("kind"); | |
| kind?.forEach((k) => params.append("kind", k)); | |
| const timeout = setTimeout(() => { | |
| router.replace(`${window.location.pathname}?${params.toString()}`, { | |
| scroll: false, | |
| }); | |
| }, 100); | |
| return () => clearTimeout(timeout); | |
| }, [kind, router]); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
tests/semdex/semdex_weaviate_test.go (1)
Line range hint
89-95: Strengthen test assertionsThe current assertion only checks if results exist without verifying their relevance to the search query "outage". Consider:
- Adding assertions to verify the search results contain the expected content
- Verifying the ordering of results
- Testing edge cases (empty query, no results)
Example improvement:
- // TODO: A better test for this lol - a.Greater(len(search1.JSON200.Items), 0) + r.Greater(len(search1.JSON200.Items), 0, "should return search results") + + // Verify first result contains the search term + firstResult := search1.JSON200.Items[0] + a.Contains( + strings.ToLower(firstResult.Content), + query, + "first result should contain the search term", + ) + + // Test empty query + emptySearch, err := cl.DatagraphSearchWithResponse(ctx, &openapi.DatagraphSearchParams{ + Q: "", + }, e2e.WithSession(ctx, cj)) + r.NoError(err) + r.Equal(http.StatusBadRequest, emptySearch.StatusCode())
| @@ -85,7 +85,7 @@ func TestSemdexWeaviate(t *testing.T) { | |||
| query := "outage" | |||
|
|
|||
| search1, err := cl.DatagraphSearchWithResponse(ctx, &openapi.DatagraphSearchParams{ | |||
| Q: &query, | |||
| Q: query, | |||
There was a problem hiding this comment.
Test coverage needed for the new search filters
The test is not verifying the new kind parameter functionality mentioned in the PR changes. Consider adding test cases that verify search results with different kind values to ensure the filter works correctly.
Here's a suggested improvement:
search1, err := cl.DatagraphSearchWithResponse(ctx, &openapi.DatagraphSearchParams{
- Q: query,
+ Q: query,
+ Kind: openapi.THREAD,
}, e2e.WithSession(ctx, cj))Also add a test case for searching without the kind parameter to verify backward compatibility.
Committable suggestion skipped: line range outside the PR's diff.
No description provided.