Conversation
trean
commented
Jan 21, 2026
trean
commented
There was a problem hiding this comment.
Pull request overview
This pull request redesigns the points filtering interface with a new two-field layout featuring syntax highlighting, autocomplete functionality, and skeleton loading states. The old SimilarSerachfield component is replaced with a modern filter system that separates similar search and payload filtering into distinct, interactive components.
Changes:
- Introduced new filter components (PointsFilter, SimilarSearchField, PayloadFilterField) with autocomplete and syntax highlighting
- Added PointCardSkeleton component for better loading UX with skeleton screens
- Refactored loading state management using requestCount to handle concurrent requests
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/Points/PointsTabs.jsx | Updated to use new PointsFilter component, added requestCount-based loading state, and integrated skeleton loading |
| src/components/Points/PointsFilter/PointsFilter.jsx | New main filter container that coordinates similar search and payload filter fields with responsive layout |
| src/components/Points/PointsFilter/SimilarSearchField.jsx | New component for similar search with chip-based UI, auto-expand behavior, and Autocomplete integration |
| src/components/Points/PointsFilter/PayloadFilterField.jsx | New payload filter with syntax highlighting, autocomplete suggestions, and keyboard navigation |
| src/components/Points/PointsFilter/StyledPointsFilter.js | Styled components and shared styling utilities for the filter components |
| src/components/Points/PointsFilter/helpers.js | Utility functions for parsing, normalizing, and processing filter inputs |
| src/components/Points/PointCardSkeleton.jsx | New skeleton loading component for point cards |
| package-lock.json | Dependency updates including version changes to MUI packages and other dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ))} | ||
| </> | ||
| )} | ||
| {points && !errorMessage && requestCount === 0 && points.points?.length === 0 && ( |
There was a problem hiding this comment.
The condition check on line 176 evaluates requestCount === 0 along with other conditions. However, this might create a race condition where points become available but requestCount is still > 0 briefly, causing the "No Points" message not to appear when it should. Consider whether the requestCount check is necessary here or if it should be part of the isLoading check instead.
| {points && !errorMessage && requestCount === 0 && points.points?.length === 0 && ( | |
| {!isLoading && points && !errorMessage && points.points?.length === 0 && ( |
| import React from 'react'; | ||
| import { Card, CardContent, CardHeader, Divider, Skeleton, Box } from '@mui/material'; | ||
|
|
||
| const PointCardSkeleton = () => { | ||
| return ( | ||
| <Card | ||
| elevation={0} | ||
| sx={{ | ||
| display: 'flex', | ||
| flexDirection: 'column', | ||
| height: '100%', | ||
| }} | ||
| > | ||
| <CardHeader | ||
| title={<Skeleton variant="text" width="60%" height={24} />} | ||
| action={ | ||
| <Box sx={{ display: 'flex', alignItems: 'center' }}> | ||
| <Skeleton variant="circular" width={32} height={32} sx={{ mr: 1 }} /> | ||
| <Skeleton variant="circular" width={32} height={32} /> | ||
| </Box> | ||
| } | ||
| /> | ||
| <CardContent sx={{ padding: '0.5rem 1rem' }}> | ||
| <Skeleton variant="rounded" height={60} sx={{ mb: 1 }} /> | ||
| <Skeleton variant="rounded" height={40} /> | ||
| </CardContent> | ||
| <Divider /> | ||
| <CardContent sx={{ padding: '1rem' }}> | ||
| <Skeleton variant="rounded" height={100} /> | ||
| </CardContent> | ||
| </Card> | ||
| ); | ||
| }; | ||
|
|
||
| export default PointCardSkeleton; |
There was a problem hiding this comment.
The new PointCardSkeleton component lacks test coverage. Given that the repository has comprehensive test coverage for other components, this new component should include unit tests to ensure it renders correctly and handles its props appropriately.
| export const calculatePopperOffset = (filterInputValue, currentWordStart) => { | ||
| // Get the text before the current word | ||
| const textBeforeWord = filterInputValue.slice(0, currentWordStart); | ||
|
|
||
| // Measure the width of text before the word using a canvas | ||
| const canvas = document.createElement('canvas'); | ||
| const ctx = canvas.getContext('2d'); | ||
| // Match the editor's font | ||
| ctx.font = '1rem system-ui, -apple-system, sans-serif'; | ||
| const textWidth = ctx.measureText(textBeforeWord).width; | ||
|
|
||
| // Add offset for the filter icon (approximately 24px for icon + margin) | ||
| const iconOffset = 28; | ||
|
|
||
| return [textWidth + iconOffset, 4]; | ||
| }; |
There was a problem hiding this comment.
The calculatePopperOffset function creates a new canvas element on every call but never cleans it up. This could lead to memory leaks if the function is called frequently. Consider caching the canvas element or using a more efficient method to measure text width, such as using a hidden DOM element with the same font styling.
| onExpandChange(false); | ||
| } else if (conditionLabels.length > MAX_VISIBLE_CHIPS) { | ||
| onExpandChange(true); | ||
| } else if (conditionLabels.length <= MAX_VISIBLE_CHIPS) { |
There was a problem hiding this comment.
The auto-expand/collapse logic has redundant conditions. Lines 53-57 check both > MAX_VISIBLE_CHIPS and <= MAX_VISIBLE_CHIPS, with the second condition being unnecessary since it's the else case. Simplify by removing the else-if condition on line 55 and just using an else block.
| } else if (conditionLabels.length <= MAX_VISIBLE_CHIPS) { | |
| } else { |
| const handleValueChange = useCallback((newValue) => { | ||
| setInputValue(newValue); | ||
| setHighlightedIndex(0); | ||
| setCursorPosition(newValue.length); |
There was a problem hiding this comment.
The handleValueChange function always sets cursorPosition to newValue.length, which means the cursor position is always placed at the end of the input when the value changes. This could be incorrect if the user is typing in the middle of the text. Consider tracking the actual cursor position from the textarea's selectionStart instead of assuming it's at the end.
| setCursorPosition(newValue.length); | |
| // Derive cursor position from the underlying textarea's selectionStart when possible | |
| setTimeout(() => { | |
| const textarea = containerRef.current?.querySelector('textarea'); | |
| if (textarea && typeof textarea.selectionStart === 'number') { | |
| setCursorPosition(textarea.selectionStart); | |
| } else { | |
| // Fallback to placing cursor at the end if selectionStart is unavailable | |
| setCursorPosition(newValue.length); | |
| } | |
| }, 0); |
| import React, { useMemo, useState, useCallback } from 'react'; | ||
| import PropTypes from 'prop-types'; | ||
| import { Box, Grid } from '@mui/material'; | ||
|
|
||
| import SimilarSearchField from './SimilarSearchField'; | ||
| import PayloadFilterField from './PayloadFilterField'; | ||
|
|
||
| const PointsFilter = ({ onConditionChange, conditions = [], payloadSchema = {}, points }) => { | ||
| const [isSimilarExpanded, setIsSimilarExpanded] = useState(false); | ||
|
|
||
| const similarConditions = useMemo(() => conditions.filter((condition) => condition.type === 'id'), [conditions]); | ||
|
|
||
| const payloadConditions = useMemo(() => conditions.filter((condition) => condition.type === 'payload'), [conditions]); | ||
|
|
||
| const payloadKeyOptions = useMemo(() => { | ||
| const keys = new Set(); | ||
| Object.keys(points?.payload_schema || {}).forEach((key) => keys.add(key)); | ||
| Object.keys(payloadSchema || {}).forEach((key) => keys.add(key)); | ||
| return [...keys]; | ||
| }, [points, payloadSchema]); | ||
|
|
||
| const handleExpandChange = useCallback((expanded) => { | ||
| setIsSimilarExpanded(expanded); | ||
| }, []); | ||
|
|
||
| return ( | ||
| <Box> | ||
| <Grid container spacing={1}> | ||
| {/* Similar search field (with chips) */} | ||
| <Grid size={{ xs: 12, md: isSimilarExpanded ? 12 : 3 }}> | ||
| <SimilarSearchField | ||
| similarConditions={similarConditions} | ||
| payloadConditions={payloadConditions} | ||
| onConditionChange={onConditionChange} | ||
| isExpanded={isSimilarExpanded} | ||
| onExpandChange={handleExpandChange} | ||
| /> | ||
| </Grid> | ||
|
|
||
| {/* Payload filter field (with syntax highlighting) */} | ||
| <Grid size={{ xs: 12, md: isSimilarExpanded ? 12 : 9 }}> | ||
| <PayloadFilterField | ||
| payloadConditions={payloadConditions} | ||
| similarConditions={similarConditions} | ||
| payloadSchema={payloadSchema} | ||
| payloadKeyOptions={payloadKeyOptions} | ||
| onConditionChange={onConditionChange} | ||
| /> | ||
| </Grid> | ||
| </Grid> | ||
| </Box> | ||
| ); | ||
| }; | ||
|
|
||
| PointsFilter.propTypes = { | ||
| conditions: PropTypes.array, | ||
| payloadSchema: PropTypes.object, | ||
| points: PropTypes.shape({ | ||
| payload_schema: PropTypes.object, | ||
| points: PropTypes.arrayOf(PropTypes.shape({ payload: PropTypes.object })), | ||
| }), | ||
| onConditionChange: PropTypes.func.isRequired, | ||
| }; | ||
|
|
||
| export default PointsFilter; |
There was a problem hiding this comment.
The new PointsFilter components (PointsFilter.jsx, SimilarSearchField.jsx, PayloadFilterField.jsx) and helper functions (helpers.js) lack test coverage. Given that the repository has comprehensive test coverage for other components (e.g., CollectionAliases, ClusterShardRow, CreateCollection), these new components should also include unit tests to ensure their functionality is properly validated.
